search

withastro / adapters

Home for Astro's core maintained adapters
46 stars 25 forks source link

netlify adapter - add esbuild plugin to allow `node:` prefixed imports #286

Open theoephraim opened 2 weeks ago

theoephraim commented 2 weeks ago

Addresses #285

Changes

Adds a simple esbuild plugin to allow node: prefixed imports in the netlify adapter's call to esbuild.

Testing

Tests are currently not passing.

I started to play with it but it seems to do more with how the testing is set up and fixtures are loaded. I would have expected the middleware having node imports in it to not trip up at all, and only falter on the unprefixed node import, only when edgeMiddleware: true option is on.

Happy to help collaborate to fix it, but someone with more knowledge of the testing setup will be much better able to solve this.

I have personally verified that adding this code fixes my issues when building and deploying to netlify, and does not seem to break anything else.

Docs

I think this change should match user's expectations moreso than the current behaviour.

changeset-bot[bot] commented 2 weeks ago

🦋 Changeset detected

Latest commit: 591950d0575df2a4abca9564fa93c0b9d7a08ffd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | ---------------------------------- | ----- | | @astrojs/netlify | Patch | | @test/netlify-hosted-astro-project | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

alexanderniebuhr commented 5 days ago

I will probably find time to look at the test soon. However wondering to get @ascorbic review of the general approach of allowing node: prefix. Should be safe, I guess :)

ascorbic commented 5 days ago

Yes, the approach is good, and is similar to the one I used for Remix. A good future addition could potentially be to automatically add the prefix for all node builtins when reolving the ID.