-
-
Notifications
You must be signed in to change notification settings - Fork 216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add babel-plugin-transform-regex to devDependencies #773
base: main
Are you sure you want to change the base?
Conversation
Thank you for creating this PR. Since I have less time at the moment and other things have a higher priority, I will try to get back to you next week. |
Is it not possible to do the transformation as part of our build step? I don't like that we need a |
No problem. I've updated the PR with a solution that generates a tsup (and esbuild, which it uses) doesn't have a built-in way to alias local file import paths, so I created a tiny build plugin in If you prefer to avoid the build plugin, an alternative to the current (new) approach could be:
But my initial thought is that moving files around like that would not be preferable. |
Thanks for the update! I am pretty busy at the moment and mainly focused on getting our v1 RC and after that our v1 release ready. I will come back to this PR when I have more time again or this feature becomes more important. |
442aa61
to
96bb4be
Compare
No problem, take your time. If you have a moment, though, I'm curious whether you have concerns with the current approach (or need time to think about it), or if you think the current PR is on the right track to pick back up from when you have more time. |
In general, I don't like to complicate the setup because I like things simple, but of course I see the benefits of this PR. Is it possible to run our tests with the compiled output? For security reasons this seems to be necessary in the long run. |
I share your concern. This was actually the primary reason for my initial approach of having a separate "source" file, with I've simplified things somewhat in the latest update to the PR, including getting rid of
I've updated the PR to do so. Vite/Rollup share the resolve.alias option, so this was easy to do. Their local aliasing support seems very robust (and allows regexes or functions, which I didn't use here). |
Thank you for the changes! This PR is now going in a good direction. |
Glad to hear it! No worries if it takes you some time to come back to this, but in the meantime, I've made a couple more tweaks and I've gone ahead and updated the PR to cover the first 12 of 24 regexes in I also included the following question/comment above // Q: Would it be tree-shakeable if this used interpolation without adding any new variables, as
// ``new RegExp(`^(?:${IPV4_REGEX.source.slice(1, -1)}|${IPV6_REGEX.source.slice(1, -1)})$`, 'iu')``? If the answer is yes, that would remove a lot of redundancy, since this is the biggest regex in the file. That redundancy is much easier to verify now that I've rewritten the regexes in a readable way. Important: I've been very careful to ensure that none of the refactored regexes changed what they match (existing bugs in |
I've rebased on top of recent conflicting Valibot configuration changes, and additionally bumped |
4666cf9
to
41ae798
Compare
7a97dd0
to
b5f7f4d
Compare
Closes #704.
Per previous discussion with @fabian-hiller in #704, this adds package babel-plugin-transform-regex to
devDependencies
for improved DX.library/src/regex.ts
aslibrary/src/regex.src.ts
.regex.ts
is now the transpiled output.regex.src.ts
, I updated only the first two regexes (BASE64_REGEX
andBIC_REGEX
) to use theregex
tag with more readable syntax (using free spacing, subroutines, a subroutine definition group, and noncapturing(…)
).regex.ts
for these two regexes is identical to before, which helps demonstrate that the Babel plugin is doing a great job with producing efficient and readable native regexes (since the regexes being replaced were hand-tuned regex literals). As a result, this PR's only change toregex.ts
is an added header comment.regex.src.ts
(apart from a hint comment at the top) is untouched.regex
todevDependencies
so the types work inregex.src.ts
.import { regex } from 'regex';
line inregex.src.ts
is stripped from transpiled source by the Babel plugin (via itsremoveImport
option which removes any import declarations with module name'regex'
).regex
work, I had to updatelibrary/tsconfig.json
, changing"moduleResolution": "node"
to"Bundler"
. Values"NodeNext"
and"Node16"
would also work forregex.src.ts
, but those values conflict with other, currenttsconfig.json
settings.