-
Notifications
You must be signed in to change notification settings - Fork 3
support modern language features #6
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
base: master
Are you sure you want to change the base?
Conversation
staltz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mixmix in spirit this is great, and I've tried to do this myself once, but got lost in the complexity that is substack's forest of modules that handle this on top of acorn. acorn itself supports modern JS but the browserify ecosystem on top of it doesn't, yet.
Your PR however is going to cause at least two different kinds of showstopper bugs.
I recommend one these three:
- Do this PR properly by updating
acornanddetectiveandresolveetc - Don't update secret-stack to 7 (it didn't receive that very important updates compared to 6)
- (My favorite) Use
esbuildinstead of noderify. I've used it for other projects and it's really really good. Not only is it flexible and does everything that noderify does, it's faster and well maintained.
| 'worker_threads', | ||
| 'zlib' | ||
| 'zlib', | ||
| 'supports-color' // debug tests to see if this is present by requiring it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big problem. Whoever uses the actual real supports-color npm package will not be able to use noderify.
If this is only for tests then there should be tests-specific flag that activate this.
| .replace(/\?\?=/g, '=') // nullish coalescing assignment | ||
| .replace(/\?\?/g, '||') // nullish coalescing | ||
| .replace(/\?\.\(/g, '(') // optional chaining (into a function) | ||
| .replace(/\?\./g, '.') // optional chaining (into a property) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ~80% likely to cause breaking changes out there. ??= cannot be replaced by = without causing bugs that are hard to detect. Example:
BEFORE
function init(opts) {
opts.timeout ??= 5*60*60;
}AFTER
function init(opts) {
opts.timeout = 5*60*60;
}Now whoever calls init({timeout: 2}) will feel super confused that the timeout ends up being 5 hours instead of the expected 2 seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this doesn't mutate the code that's exported. I confirmed that in the bundle.
This only mutates the code that detective is looking at for require commands
Trying to
noderifysecret-stack@7we get errors fromacorntrying to parse??=,??,cb?.(),opts?.timeoutetcI've looked through the stack for the best solution and tried:
ecmaVersion: 2023. supposedly supported by the version ofacornwe're using, but nopedetectfunction to "tidy" the new features into version which don't break the parserI also tidied logging to make it easier to see at a glance if things are passing