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

ESM support: can't recognize '--experimental-specifier-resolution' flag passed via NODE_OPTIONS #1072

Closed
koshic opened this issue Jul 4, 2020 · 2 comments · Fixed by #1085
Labels
bug you can do this Good candidate for a pull request.

Comments

@koshic
Copy link

koshic commented Jul 4, 2020

Code at

uses process.execArgv to get options and doesn't analyze NODE_OPTIONS anyhow.

@cspotcode cspotcode added bug you can do this Good candidate for a pull request. labels Jul 5, 2020
@cspotcode
Copy link
Collaborator

I'm marking this as "help wanted" to indicate that we'll wait for a pull request. This seems like a straightforward contribution.

node-esm-resolve-implementation.js is largely copied from node's own source code.

// Copied from https://raw.githubusercontent.com/nodejs/node/v13.12.0/lib/internal/modules/esm/resolve.js
// Then modified to suite our needs.
// Formatting is intentionally bad to keep the diff as small as possible, to make it easier to merge
// upstream changes and understand our modifications.

To write an ESM loader, we need to mimic many built-in behaviors of node. Unfortunately node does not expose this functionality via an API. So our best option for maintainability is to copy-paste node's own implementation where appropriate, tweaking as needed.

See also: discussion in #1010, where @evg656e helped me implement our execArgv parsing.

@evg656e
Copy link

evg656e commented Jul 6, 2020

Yep, I missed the need to parse the NODE_OPTIONS environment variable in the initial implementation.

Unfortunately, the arg parser does not allow getting options of the environment variables (like 'yargs` .env function does).

The workaround to use ParseNodeOptionsEnvVar from node source.

I rewrote getOptionValue code in my branch.

As you can see, the code becomes quite messy, so it may be worth putting it into a separate file to keep main module clean.

cspotcode added a commit that referenced this issue Jul 29, 2020
@cspotcode cspotcode mentioned this issue Jul 29, 2020
cspotcode added a commit that referenced this issue Aug 4, 2020
* Fix #1072

* add tests
@cspotcode cspotcode mentioned this issue Aug 21, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug you can do this Good candidate for a pull request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants