Skip to content

Conversation

@JakeChampion
Copy link
Contributor

The current edge-bundler attempts to import a dependency via it's package-name, for example if the edge-function was importing @secret/magic/sdk/meow, then the edge-bundler would attempt to import @secret/magic during it's bundling process. This hits an issue if the dependency being imported is using the packge.json.exports field and has not allowed the bare package-name to be imported.

We can see this issue with package's such as @modelcontextprotocol/sdk which has an exports field defined like so:

"exports": { "./*": { "import": "./dist/esm/*", "require": "./dist/cjs/*" } }, 

This patch reimplements the edge-bundler's logic so that it uses the same import as that used within the edge-function itself, going back to our previous example of importing @secret/magic/sdk/meow in the edge-function, the edge-bundler will now also import @secret/magic/sdk/meow during it's bundling process. This solves the issue mentioned about a package not exporting it's bare package-name.

The patch has changed how edge-bundler parses edge-functions for their imports, it no longer uses vercel/nft as that package finds the files being used, but the files may not be directly importable due to a package.json.exports field definition. This has been replaced with parse-imports, which does a lexical scan of the files to find the import tokens. This approach is useful in that we can now detect all the different types of imports, such as if they are static or dynamic imports. For dynamic imports we can also detect if the import-specifier is a constant or not - this is useful because if it is a constant then we are able to bundle it, if it is not a constant, we could decide to report that the import may not work.
The previous approach using vercel/nft did have the ability to find paths which are used but not imported, such as paths used within fs.readFile etc, vercel/nft would mark those as "assets". edge-bundler would keep track of the dependencies which had "assets" and mark them as containing 'extraneous files'.
The CLI, if invoked with the dev command would then print out a note like this:

The following npm modules, which are directly or indirectly imported by an edge function,
may not be supported: dictionary. For more information, visit https://ntl.fyi/edge-functions-npm.

This note was only shown for the dev command and not for build or deploy, a build and a deployment were still possible to take place.

This patch has not implemented this note for the dev command, I don't think this is an issue.

@JakeChampion JakeChampion force-pushed the jake/efs branch 7 times, most recently from b6c2857 to 3802e0f Compare March 28, 2025 11:18
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@netlify netlify deleted a comment from github-actions bot Mar 28, 2025
@github-actions
Copy link
Contributor

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@github-actions
Copy link
Contributor

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Apr 2, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

2 similar comments
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

…ithin Netlify Edge Functions The current edge-bundler attempts to import a dependency via it's package-name, for example if the edge-function was importing `@secret/magic/sdk/meow`, then the edge-bundler would attempt to import `@secret/magic` during it's bundling process. This hits an issue if the dependency being imported is using the packge.json.exports field and has not allowed the bare package-name to be imported. We can see this issue with package's such as `@modelcontextprotocol/sdk` which has an exports field defined like so: ``` "exports": { "./*": { "import": "./dist/esm/*", "require": "./dist/cjs/*" } }, ``` This patch reimplements the edge-bundler's logic so that it uses the same import as that used within the edge-function itself, going back to our previous example of importing `@secret/magic/sdk/meow` in the edge-function, the edge-bundler will now also import `@secret/magic/sdk/meow` during it's bundling process. This solves the issue mentioned about a package not exporting it's bare package-name. The patch has changed how edge-bundler parses edge-functions for their imports, it no longer uses vercel/nft as that package finds the files being used, but the files may not be directly importable due to a package.json.exports field definition. This has been replaced with `parse-imports`, which does a lexical scan of the files to find the import tokens. This approach is useful in that we can now detect all the different types of imports, such as if they are static or dynamic imports. For dynamic imports we can also detect if the import-specifier is a constant or not - this is useful because if it is a constant then we are able to bundle it, if it is not a constant, we could decide to report that the import may not work. The previous approach using vercel/nft did have the ability to find paths which are used but not imported, such as paths used within `fs.readFile` etc, vercel/nft would mark those as "assets". edge-bundler would keep track of the dependencies which had "assets" and mark them as containing 'extraneous files'. The CLI, if invoked with the `dev` command would then print out a note like this: >The following npm modules, which are directly or indirectly imported by an edge function, >may not be supported: dictionary. For more information, visit https://ntl.fyi/edge-functions-npm. This note was only shown for the `dev` command and not for `build` or `deploy`, a build and a deployment were still possible to take place. This patch has not implemented this note for the `dev` command, I don't think this is an issue.
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@JakeChampion JakeChampion marked this pull request as ready for review April 3, 2025 14:21
@JakeChampion JakeChampion requested a review from a team as a code owner April 3, 2025 14:21
@JakeChampion JakeChampion enabled auto-merge (squash) April 8, 2025 17:48
@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2025

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@JakeChampion JakeChampion merged commit aab5b2d into main Apr 8, 2025
32 of 33 checks passed
@JakeChampion JakeChampion deleted the jake/efs branch April 8, 2025 19:21
@ndhoule
Copy link
Contributor

ndhoule commented Apr 8, 2025

@JakeChampion Not sure if you're aware, but if not: Heads up that this changes @netlify/edge-bundler public interface in a way that breaks the CLI. Are you working on an accompanying PR to that repo?

@JakeChampion
Copy link
Contributor Author

@JakeChampion Not sure if you're aware, but if not: Heads up that this changes @netlify/edge-bundler public interface in a way that breaks the CLI. Are you working on an accompanying PR to that repo?

I am 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment