fix: upgrade precinct

[netlify-team-account-1]

- Summary

closes #743

- Test plan

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

[netlify-team-account-1]

@eduardoboucas one way to fix this issue is to always use esbuild instead of precinct. What's the state of the feature flag, can we try rolling it out to more users? (maybe only targeting SvelteKit projects at first, cc @benmccann)

[benmccann]

I'd prefer not to require users to bundle their projects with esbuild. SvelteKit uses esbuild right now and I'm trying hard to remove it because it's led to a number of issues. See sveltejs/kit#2580. I'd really like if we could make zip-it-and-ship-it work

[netlify-team-account-1]

Thank you for bringing that up! The issues mentioned in that PR seem to be related to cases where ESBuild is the first transpilation (e.g. from TypeScript to JavaScript). In the case of Svelte on Netlify, ESBuild would work on already-bundled files, which makes the source-map / decorator metadata issues more unlikely. cc'ing @eduardoboucas for more context on this.

[benmccann]

Hmm, I'm not sure if that's generally true. SvelteKit bundles everything with Vite first. It then uses ESBuild for a final bundling

Is there a reason we can't add an option to continue if the package cannot be found? Or alternatively, or in addition, we could assume packages with a node: prefix are provided by Node

[netlify-team-account-1]

I opened a PR on the dependency that does the module detection, to add node: as an indicator for built-in packages. Let's see what they say!

[netlify-team-account-1]

the PR was merged 🥳 @eduardoboucas could you take a look at this?

[github-actions]

⏱ Benchmark results

largeDepsNft: 1m 0.4s

^ 1m 0.4s 
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T    

Legend

• T (current commit): 1m 0.4s

largeDepsZisi: 1m 9s

^  1m 9s  
│   ┌──┐  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
│   |▒▒|  
└───┴──┴──>
     T    

Legend

• T (current commit): 1m 9s

[benmccann]

@netlify-team-account-1 thanks for your help with this! the tests are failing - is that to be expected?

[netlify-team-account-1]

thanks for your help with this! the tests are failing - is that to be expected?

accidentally enabled that test to run on esbuild. tests should go green now :)

[eduardoboucas]

Sorry for the delay in getting this reviewed!

Added a non-blocking suggestion, otherwise LGTM. 🚀

[eduardoboucas]

We're not really ignoring these imports, we're just treating them as imports for built-in modules, right?

Also added two test variations:

• bundler_default_nft
• todo:bundler_esbuild (so support in esbuild is on our radar)

Suggested change

	testMany('Ignores node:-prefixed imports (repro #743)', ['bundler_default', 'bundler_nft'], async (options, t) => {
	testMany('Handles imports of built-in modules with the `node:` prefix', ['bundler_default', 'bundler_default_nft', 'bundler_nft', 'todo:bundler_esbuild'], async (options, t) => {

[eduardoboucas]

By the way, #698 adds a test for the node: prefix. It uses fs rather than stream/web, which might be a better test because it doesn't rely on Node 16.

We could let #773 upgrade precinct and then merge #698 for the test.

@netlify-team-account-1 what do you think?

[netlify-team-account-1]

Sounds good 👍

[netlify-team-account-1]

I'll close this PR as per #746 (comment). @benmccann the node: import issue was resolved by #773, which was published in v4.28.1. It'll come to Netlify CLI via netlify/cli#3533.

[benmccann]

Thanks for the fix here! What's needed to get netlify/cli#3533 merged? I notice that there's currently a dozen different PRs open to upgrade different dependencies and am not sure if there's some process you have for batching them all or something along those lines?