-
Notifications
You must be signed in to change notification settings - Fork 145
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
Improve ESM detection #173
Conversation
const loader = new Loader({requireShim, importShim}); | ||
|
||
const loaderPromise = loader.load('./foo/bar/baz.mjs'); | ||
await expectAsync(loader.load('./foo/bar/baz.mjs')).toBeResolved(); |
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.
Had to wait for completion here, because importShim
is not called immediately anymore. I have to wait for the requireShim
to error, before importShim
is even called.
Thanks for your interest in this and the PR. I'm not convinced that this is the right way to detect ES modules. I worry about taking an approach that's so different from how Node determines which module type to use, even if we're theoretically leaving the decision up to Node. I'm specifically concerned about the potential impact on modules that are designed to work as either ES or CommonJS modules. At this point I don't have a solid enough understanding of that area to evaluate the impact of this change. I'd also prefer to avoid throwing and catching large numbers of exceptions, especially during startup. Each additional thrown exception makes it harder to use exception breakpoints as a debugging tool. On balance, I think we're probably better off doing what Node actually does, as I understand it: walk up the filesystem from the module to be loaded until a package.json is found, then inspect that. One additional piece of feedback, for this and your other PR: Please add and update tests as appropriate so that the test suite verifies and accurately describes the new functionality. E.g. in this case the loader spec should describe the new behavior, but it still says that non-.mjs files are loaded as commonjs modules. We rely on the tests both to help document things and to let us know if our changes have broken anything, so high-quality test coverage is important. |
Thanks for your reply and the feedback. You mention "modules that are designed to work as either ES or CommonJS". I am not sure whether these actually exists. Can you give an example? As far as I understand, a module (i.e. file) can never be both. The default for an empty file is to be recognised as CommonJS. As soon as there is a single Actually, I have read about people adding an empty export ( Maybe you are thinking of packages that are designed to work as either ES or CommonJS? (i.e. a collection of files) I think those are implemented by compiling two different versions of each package entry point. Your point about the test names needing to be updated is 100% valid. I would be more than happy to improve these if there is a general willingness to merge, which does not seem to be the case. |
@sgravrock, I thought I would take a moment to address the rest of your previous comment. You expressed concern with the detection method relying on an exception thrown by Node. I have had those concerns myself and I wasn't sure which one was going to be the better approach: #170 (comment) The reason why I decided to use the exception is because I deemed it to be the less error-prone method of the two. Having to stay in sync with node's detection algorithm might be a lot of extra work, that could be out of scope for this project? The Node.js resolution is not so simple (see the docs for Maybe a third option would be to wait for the end-of-life of Node.js 10 (scheduled for April 2021). I assume at that point you will drop official support for v10, right? Starting with v12 you no longer need |
Let's use the parent issue to discuss how to improve ES module detection, so we can keep the conversation in one place. |
Closing in favor of da3ecaa, which I believe covers the same functionality. Thanks for your work on this PR and your help figuring out how best to support ES modules. |
Hi,
I have created a pull request for issue #170 which is marked as "help needed".
Please let me know if you have any comments.
Thanks for creating Jasmine, btw!