Forward whole context to ESM hooks #1572
Conversation
Ensure that all properties of the context objects passed to the `resolve` and `load` ESM hooks are forwarded to the default hooks. This change is necessary to for using [import assertions in Node v17][1]. Node v17 requires these import assertions for JSON modules. If the assertions are not passed down to the default `load` function, Node refuses to import the JSON module. To prevent similar issues we also forward the context for the `resolve` hook. [1]: https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#import-assertions Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
|
Thanks, is this similar to #1559? If so, can we collaborate to create a single pull request with all the necessary tests? There are no hard and fast rules here, as long as we end up with good tests and a good PR that we can merge, everyone will get attribution in the release notes. #1559 has some tests, but I'm not sure if they are sufficient; I haven't had enough time to review. If you think it is appropriate to combine these two pull requests, can you offer a code review? Hopefully @Pokute is available to discuss as well. It's the holiday season for me so my time is limited, but hopefully we can merge and release a bunch of things near the beginning of the year. EDIT: fixed links to the correct PR |
Yes it is and I’ve added my comments to the PR. (The PR somehow escaped my notice). My PR also forwards the context for the |
|
Closing in favor of #1559 |
Ensure that all properties of the context objects passed to the
resolveandloadESM hooks are forwarded to the default hooks.This change is necessary to for using import assertions in Node v17. Node v17 requires these import assertions for JSON modules. If the assertions are not passed down to the default
loadfunction, Node refuses to import the JSON module.To prevent similar issues we also forward the context for the
resolvehook.I’d love to add some tests for this but I’d need some help as to how to approach
this.