Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

fix: esm imports with node: prefix on esbuild #802

Merged
merged 4 commits into from
Nov 11, 2021
Merged

fix: esm imports with node: prefix on esbuild #802

merged 4 commits into from
Nov 11, 2021

Conversation

netlify-team-account-1
Copy link
Contributor

- Summary

fixes a bundling error that occurs when esbuild bundles a node:-namespaced esm import.

cc nuxt/nuxt#12764

- Test plan

- A picture of a cute animal (not mandatory but encouraged)

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Nov 11, 2021
@netlify-team-account-1 netlify-team-account-1 marked this pull request as ready for review November 11, 2021 10:33
@netlify-team-account-1 netlify-team-account-1 mentioned this pull request Nov 11, 2021
Closed
eduardoboucas
eduardoboucas reviewed Nov 11, 2021
View reviewed changes
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Added just a tiny comment.

src/runtimes/node/bundlers/esbuild/bundler.ts Outdated Show resolved Hide resolved
eduardoboucas
eduardoboucas approved these changes Nov 11, 2021
View reviewed changes
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎸

@kodiakhq kodiakhq bot merged commit 199f6ed into main Nov 11, 2021
@kodiakhq kodiakhq bot deleted the esbuild-node-prefixed-imports branch November 11, 2021 11:09
@github-actions
Copy link
Contributor

⏱ Benchmark results

largeDepsEsbuild: 7.7s

^   7.7s  
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T
Legend
  • T (current commit): 7.7s

largeDepsNft: 50.2s

^  50.2s  
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T
Legend
  • T (current commit): 50.2s

largeDepsZisi: 1m 4.5s

^ 1m 4.5s 
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T
Legend
  • T (current commit): 1m 4.5s

@hrishikesh-k
Copy link

hrishikesh-k commented Nov 11, 2021 •
edited

Hey team, is this supposed to be working fine in Netlify CLI?

I was testing a repo and got the exact same error in CLI, except the user is using Vite and not Nuxt.js.

EDIT: Their netlify.toml is using esbuild, so it doesn't seem like a zip-it-ship-it problem. I'll take this to the CLI team.

@netlify-team-account-1
Copy link
Contributor Author

It's indeed a ZISI problem, which uses esbuild internally. It's expected that this error occurs in the CLI until we ship a new version of that.

Skn0tt pushed a commit to netlify/build that referenced this pull request May 21, 2024
@netlify-team-account-1
fix: esm imports with node: prefix on esbuild (netlify/zip-it-and-shi…
12daf7c 
…p-it#802)

* chore: add reproduction for esbuild and node-prefixed imports

* fix: mark node:-prefixed modules as external for esbuild

* chore: move builtin-logic into its own plugin

* chore: run nodebuiltin plugin first

Co-authored-by: Netlify Team Account 1 <netlify-team-account-1@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eduardoboucas eduardoboucas eduardoboucas approved these changes

Assignees
No one assigned
Labels
automerge type: bug code to address defects in shipped code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@netlify-team-account-1 @hrishikesh-k @eduardoboucas