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

add support for initialize hook #240

Merged
merged 29 commits into from
Sep 11, 2023
Merged

Conversation

iambumblehead
Copy link
Owner

this PR adds support for the initialize hook: https://nodejs.org/api/esm.html#initialize

Essentially, with node v20.6 esmock does not need to be called with "--loader" and instead esmock creates its own message channel and script-registers the loader to the channel itself.

@iambumblehead
Copy link
Owner Author

The newer initialize hook will really simplify many things and allow us to remove global variables and other things. For now though, things will get complicated.

@iambumblehead
Copy link
Owner Author

This PR has a few major issues but is almost working. Observe current issues with esmock.node.global.test.js

./tests/tests-node/

npm run test-one esmock.node.global.test.js
npm run test-one-no--loader esmock.node.global.test.js

Main problem: currently, loader hooks engage three times and process files three times. This happens when "initialized" loader exports hooks used by "--loader" loader. If we did not need backward-compatibility, places needed by "--loader" could be removed and we would be done,

  • 'esmock.js' should not export hook definitions unless "--loader" is used export * from './esmockLoader.js'
  • module.register should only be called one time by the top level caller, if no loader is already introduced by "--loader"
  • the error message should explain --loader is used with older versions of node

@iambumblehead
Copy link
Owner Author

yikes node v20's globalPreload mechanism esmock uses to message the loader was removed this morning! nodejs/node#49144

@iambumblehead
Copy link
Owner Author

iambumblehead commented Sep 9, 2023

cautiously, this branch is mostly working... other things to consider adding here before this branch will be ready,

  • update the README, remove package.json example and include some message that links to the wiki for demonstrations of "--loader" that are required by older versions of node
  • update the error message that is thrown when loader is undefined, message something like "node versions earlier than v18.6 require the --loader hook; for more details see https://github.com/iambumblehead/esmock/wiki"
  • esmockLoader.js should be included and not-deleted from the published directory of files
  • tests verifying "import.meta.resolve" should be removed and possibly replaced with tests verifying esmock without a --loader when node version is greater than 20.6
  • we should try re-enabling the latest node 18 for the unit-tests, in case any changes are back-ported to appear in newer versions of node 18. If the edge-case windows test fails there we should consider disable that specific test when for windows and node version 18.

@iambumblehead
Copy link
Owner Author

iambumblehead commented Sep 10, 2023

package.json scripts in the test folders are updated with patterns like this one used by the mocha folder

"scripts": {
  "isnodelt20_6": "node -e \"(([mj, mn]) => (+mj < 20 || (+mj === 20 && +mn < 6)))(process.versions.node.split('.')) || process.exit(1)\"",
  "test:loader": "mocha --loader=esmock",
  "test:current": "mocha",
  "test": "npm run test:loader && npm run isnodelt20_6 || npm run test:current"
}

This is kind of noisy however works well with windows and npm and, for newer versions of node, runs tests without --loader=esmock. When support for node18 is dropped, a lot of noise can be removed from the sources and the script pattern can simplified to just this,

"scripts": {
  "test": "mocha"
}

please review :)

@iambumblehead
Copy link
Owner Author

Planning to merge and publish this soon so changes will be available for the workspace PR #243

@iambumblehead iambumblehead merged commit 8cb00f1 into master Sep 11, 2023
8 checks passed
@iambumblehead iambumblehead deleted the add-support-for-initialize-hook branch September 11, 2023 17:28
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

Successfully merging this pull request may close these issues.

2 participants