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

refactor: use node: prefix for imports #1346

Merged
merged 4 commits into from Oct 26, 2021

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Oct 21, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain: uses node: prefix for node imports

What changes did you make? (provide an overview)

Which issue (if any) does this pull request address?

Is there anything you'd like reviewers to know?

depending on: #1348, although there's a bit more linting failing. I'll push it on top of the linting PR.

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Oct 25, 2021

should remove // eslint-disable-next-line node/prefer-promises/dns or switch to dns.promises.lookup

@dnalborczyk
Copy link
Contributor Author

@dnalborczyk dnalborczyk commented Oct 26, 2021

should remove // eslint-disable-next-line node/prefer-promises/dns or switch to dns.promises.lookup

I guess the linter had a non-semver update. 😄
I removed the eslint-disable line as the HttpAgent seems to expect a callback lookup function.

LinusU
LinusU approved these changes Oct 26, 2021
Copy link
Member

@LinusU LinusU left a comment

Neat 👍

@jimmywarting jimmywarting merged commit 47d9cde into node-fetch:main Oct 26, 2021
8 checks passed
@dnalborczyk dnalborczyk deleted the node-prefix branch Oct 26, 2021
@mattfysh
Copy link

@mattfysh mattfysh commented Nov 9, 2021

this appears to have broken node-fetch when running on AWS lambda, I don't believe they are currently running a version of node which supports the node: prefix

@dnalborczyk
Copy link
Contributor Author

@dnalborczyk dnalborczyk commented Nov 9, 2021

this appears to have broken node-fetch when running on AWS lambda, I don't believe they are currently running a version of node which supports the node: prefix

aws also does not yet support esm for that matter either. are you using a bundler (webpack, rollup etc.) and outputting to commonjs?

@feross
Copy link

@feross feross commented Dec 21, 2021

Question: What is the reason to do this? What does it improve? Asking because it broke my webpack build (which does not understand the node: prefix).

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Dec 21, 2021

@feross

It begun with import streams from 'streams/web'
... it was so new that their own build tool did not yet understand that this new module was coming from nodejs itself. it was resolved when we added a node prefix, so it did know when/if there was a core node module. node-fetch/fetch-blob#122

So we added node prefix everywhere

v16.0.0, v14.18.0 Added node: import support to require(...).
v14.13.1, v12.20.0 Added node: support: v14.13.1, v12.20.0

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Dec 21, 2021

it looks like webpack has added support for node prefix webpack/webpack#12693

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.

None yet

5 participants