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

feat!: support package.json.exports field for npm dependencies used within Netlify Edge Functions #6167

Merged
merged 3 commits into from
Apr 8, 2025

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
Copy link
Contributor

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

Copy link
Contributor

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

1 similar comment
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.

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.

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
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.

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.
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.

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
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.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants