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

working mock test #39240

Closed
wants to merge 19 commits into from
Closed

working mock test #39240

wants to merge 19 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Jul 2, 2021

This PR adds a communications channel for loaders to utilize across global preload code and hooks. It allows communication from the loader directly to references. Currently it is somewhat awkward to use since MessagePort does async events but does solve the global usage in https://dev.to/giltayar/mock-all-you-want-supporting-es-modules-in-the-testdouble-js-mocking-library-3gh1 .

The important bit of this PR is that we add a test for mocking via a reflective API on the main thread.

It does alter the API signature of getGlobalPreloadCode and adds a new implicit parameter to change the import.meta initialization, this can be used to fix the problem with HTTPS imports being done via a loader by allowing a rewrite of the import.meta to the proper value instead of the cache key in the module map.

Likely we should iterate on this API since this is more the bare minimum to make things work and not a pleasant API currently.

CC: @nodejs/loaders @giltayar

@bmeck bmeck requested review from jkrems and guybedford July 2, 2021 21:02
@github-actions github-actions bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Jul 2, 2021
@giltayar
Copy link
Contributor

giltayar commented Jul 3, 2021

Looks interesting. Two questions:

  1. If I understand correctly, the mocking test code doesn't really need setImportMetaCallback, and it could do without. While this capability could be interesting, I'm not sure why it's needed (you said something about an http loader?).

  2. While the port to communicate is interesting, communicating via global variables (while admittedly ugly), is still easier to work with, and synchronous in nature. So why add this capability? My guess is that it's for a future where the loader will be in a worker, which is fine. But in that case, the messageport communication won't help deal with the problem that the "client" (the mock function) needs to pass functions (the mock function) to the loader, which cannot be done (AFAIK) using messages (because what's passed needs to be serialized).

@bmeck
Copy link
Member Author

bmeck commented Jul 3, 2021

If I understand correctly, the mocking test code doesn't really need setImportMetaCallback, and it could do without. While this capability could be interesting, I'm not sure why it's needed (you said something about an http loader?

setImportMetaCallback in the test is being used to create a 1st class channel from the preload code and the module instances. You can see in the generateModule output that it is reading the references off of import.meta.mock.

Per the HTTPS loader PR the actual behavior of import.meta.url on the web differs from files and recreating HTTPS semantics is not currently possible with a loader without manual prefixing of modules with things like import.meta.url = ... new value here ...;\n. See https://github.com/nodejs/node/pull/36328/files#diff-b1de5e9ce1e9b411001c63d11a5092dbba32e1586269a20dc11408375eb2f16cR135-R137

While the port to communicate is interesting, communicating via global variables (while admittedly ugly), is still easier to work with, and synchronous in nature. So why add this capability? My guess is that it's for a future where the loader will be in a worker, which is fine. But in that case, the messageport communication won't help deal with the problem that the "client" (the mock function) needs to pass functions (the mock function) to the loader, which cannot be done (AFAIK) using messages (because what's passed needs to be serialized).

I actually think the problem here is in part my using a MessagePort which wants to asynchronously dispatch events. I generally think we need to move loaders off thread and have faced pushback in the past due to startup penalty of doing so; however, I wrote this with off-thread in mind. Using globals is easier! However, this avoids leaking globals and allows communication to be done in a way that cannot be tampered with or prevented by user code between Loader <-> Preload Code <-> Module Instances. Definitely want to make the API work better so might have to use a different communications API than MessagePort for now. Currently the functions are never passed to the loader via a 1st class reference, they are only passed as a list of exports and the global preload code is managing the first class references. All communication with the loader is done via message ports.

lib/internal/modules/esm/initialize_import_meta.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/mock-loader.mjs Outdated Show resolved Hide resolved
@DerekNonGeneric DerekNonGeneric added the loaders Issues and PRs related to ES module loaders label Jul 3, 2021
@giltayar
Copy link
Contributor

giltayar commented Jul 4, 2021

setImportMetaCallback in the test is being used to create a 1st class channel from the preload code and the module instances. You can see in the generateModule output that it is reading the references off of import.meta.mock.

Yes, but it has enough information to just use mockedModules by itself, without recourse to using meta.mock, no?

recreating HTTPS semantics is not currently possible with a loader without manual prefixing of modules with things like import.meta.url = ...

🙏

Currently the functions are never passed to the loader via a 1st class reference, they are only passed as a list of exports and the global preload code is managing the first class references. All communication with the loader is done via message ports.

Of course you're right. And to think I wrote something like this... (well, it was a while ago 😊). So, OK, removing my objection on thread-separated loaders.

@bmeck
Copy link
Member Author

bmeck commented Sep 16, 2021

now that consolidation PR is landed, rebasing, looks like we need a slight alteration to handle multiple nesting importMetaCallbacks

doc/api/esm.md Outdated Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Sep 24, 2021

removing draft status since there aren't any pending design comments and hoping to land early next week

@bmeck bmeck marked this pull request as ready for review September 24, 2021 15:33
@bmeck bmeck changed the title wip: working mock test working mock test Oct 27, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

doc/api/esm.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Oct 28, 2021

Reading through CI reliability and the failed CI runs, I think the cause for the failures isn't this PR.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/mock-loader.mjs Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/mock-loader.mjs Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/mock-loader.mjs Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

bmeck added a commit that referenced this pull request Nov 29, 2021
PR-URL: #39240
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@bmeck
Copy link
Member Author

bmeck commented Nov 29, 2021

Landed in 1998e83

@bmeck bmeck closed this Nov 29, 2021
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 29, 2021
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
PR-URL: #39240
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
PR-URL: #39240
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #39240
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants