Skip to content
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

Don't use parser for Node v22? #89

Closed
timfish opened this issue May 29, 2024 · 4 comments
Closed

Don't use parser for Node v22? #89

timfish opened this issue May 29, 2024 · 4 comments

Comments

@timfish
Copy link
Contributor

timfish commented May 29, 2024

Testing on top of PR #85, I found that for Node v22, the parser doesn't appear to be required and we can revert back to the faster path of import(srcUrl).then(Object.keys).

We would need the parser to support v18.19 -> v21.

We could reduce the potential bundle size caused by the parser (~100KB) by:

  • Moving the parser to it's own package (eg. 'import-in-the-middle-parser')
  • Set package.json#engines to the Node versions where it's required
  • Make it an optionalDependency of import-in-the-middle
  • import('import-in-the-middle-parser') in hook.js in a try/catch

When installed with Node versions requiring the parser, the optional dependency will be installed and used.
When installed with Node v22+, the optional dependency will be missing and will not be included in any bundle

@mohd-akram
Copy link
Contributor

From my testing, the parser is only required for NODE_MAJOR == 20 && NODE_MINOR < 6. However, it's not possible to use libraries with circular dependencies (like openai) without the parser. That's why it fails below 18.19 and 20 (the threshold used to activate the parser). We seem to expect circular dependencies to fail based on v14-assert-cyclical-dependency-failure.mjs (even though regular Node.js import succeeds) so maybe this isn't a problem. Note that it fails with a code 13 error and there's some explanation for that in nodejs/node#44601.

@timfish
Copy link
Contributor Author

timfish commented May 30, 2024

Regular Node.js import can handle cyclic dependencies but dynamic import cannot.

Yeah it really is only the openai test that fails and needs the parser.

It's strange that the same test now passes in v22. I can't find anything in the release notes that would suggest this limitation has been removed.

@mohd-akram
Copy link
Contributor

Regular Node.js import can handle cyclic dependencies but dynamic import cannot.

That doesn't seem to be strictly true. Adding const openai = await import('openai') runs fine in regular Node.js. The problem seems to happen if you do that inside the cycle, which I guess because import-in-the-middle is acting as a loader, that counts.

It's strange that the same test now passes in v22.

It's failing but with a 0 exit code :) If you add a console.log after the import it doesn't reach there.

@timfish
Copy link
Contributor Author

timfish commented May 30, 2024

It's failing but with a 0 exit code :) If you add a console.log after the import it doesn't reach there.

Ah that's a shame, I missed that entirely and only noticed the tests all passing 😭

The problem seems to happen if you do that inside the cycle, which I guess because import-in-the-middle is acting as a loader, that counts.

That means we could potentially perform await import('lib') in a worker that isn't using the import-in-the-middle loader to get the exports but I guess the performance would suck. In some quick testing, the parser looks quicker than import() anyway in Node v22. I guess it's only parsing and Node is evaluating on top of that 🤷‍♂️

@timfish timfish changed the title Don't use parser for Node v22 Don't use parser for Node v22? May 30, 2024
@timfish timfish closed this as completed May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants