-
Notifications
You must be signed in to change notification settings - Fork 73
fix: inline optional dependencies when bundling the server #325
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
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
b37b907
fix: inline optional dependencies when bundling the server
vicb f427917
fixup! skip fetch cache test
vicb 7177f36
Update packages/cloudflare/src/cli/build/patches/plugins/optional-dep…
vicb ca4c87c
Update packages/cloudflare/src/cli/build/patches/plugins/require.ts
vicb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@opennextjs/cloudflare": patch | ||
--- | ||
|
||
fix: inline optional dependencies when bundling the server | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
153 changes: 0 additions & 153 deletions
153
packages/cloudflare/src/cli/build/patches/ast/optional-deps.spec.ts
This file was deleted.
Oops, something went wrong.
48 changes: 0 additions & 48 deletions
48
packages/cloudflare/src/cli/build/patches/ast/optional-deps.ts
This file was deleted.
Oops, something went wrong.
63 changes: 63 additions & 0 deletions
63
packages/cloudflare/src/cli/build/patches/plugins/optional-deps.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/** | ||
* ESBuild plugin to handle optional dependencies. | ||
* | ||
* Optional dependencies might be installed by the application to support optional features. | ||
* | ||
* When an optional dependency is installed, it must be inlined in the bundle. | ||
* When it is not installed, the plugin swaps it for a throwing implementation. | ||
* | ||
* The plugin uses ESBuild built-in resolution to check if the dependency is installed. | ||
*/ | ||
|
||
import type { OnResolveResult, PluginBuild } from "esbuild"; | ||
|
||
export function handleOptionalDependencies(dependencies: string[]) { | ||
// Regex matching either a full module ("module") or a prefix ("module/...") | ||
const filter = new RegExp( | ||
`^(${dependencies.flatMap((name) => [`${name}$`, String.raw`${name}\/`]).join("|")})` | ||
vicb marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
); | ||
|
||
const name = "optional-deps"; | ||
const marker = {}; | ||
const nsMissingDependency = `${name}-missing-dependency`; | ||
|
||
return { | ||
name, | ||
|
||
setup: async (build: PluginBuild) => { | ||
build.onResolve({ filter }, async ({ path, pluginData, ...options }): Promise<OnResolveResult> => { | ||
// Use ESBuild to resolve the dependency. | ||
// Because the plugin asks ESBuild to resolve the path we just received, | ||
// ESBuild will ask this plugin again. | ||
// We use a marker in the pluginData to break the loop. | ||
if (pluginData === marker) { | ||
return {}; | ||
} | ||
const result = await build.resolve(path, { | ||
...options, | ||
pluginData: marker, | ||
}); | ||
|
||
// ESBuild reports error when the dependency is not installed. | ||
// In such a case the OnLoad hook will inline a throwing implementation. | ||
if (result.errors.length > 0) { | ||
return { | ||
path: `/${path}`, | ||
namespace: nsMissingDependency, | ||
pluginData: { name: path }, | ||
}; | ||
} | ||
|
||
// Returns ESBuild resolution information when the dependency is installed. | ||
return result; | ||
}); | ||
|
||
// Replaces missing dependency with a throwing implementation. | ||
build.onLoad({ filter: /.*/, namespace: nsMissingDependency }, ({ pluginData }) => { | ||
return { | ||
contents: `throw new Error('Missing optional dependency "${pluginData.name}"')`, | ||
}; | ||
}); | ||
}, | ||
}; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this changeset could use some extra info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog is mostly for users - I don't think we should describe too much about the implementation details.
But I can add more details to the PR description if you feel like something is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolving the convo to merge, but feel free to send your feedback and I'll update the description.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nono the PR description has everything
What I meant is that if I were a user and I saw:
fix: inline optional dependencies when bundling the server
I don't think I would understand what that means/fixes, and I would have questions such as:Not a huge deal anyways (and you merged as I was writing this comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most users do not care about the changelog and how this adapter works. They just want their app to work.
Adding too many info to the changelog might make it more difficult for users to find what they are interested in: the changes they need to do to their app after they update.
IMO
is only of interest for power users - they would know how to find the PR (from the changelog) and check the description and/or the comments in the code.
That being said, one thing I should have mentioned is how this issue manifest (i.e. the generated error).
That's my 2 cents but feel free to add an item to the weekly agenda if you think we should discuss more about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most users do not care about the changelog and how this adapter works. They just want their app to work.
Adding too many info to the changelog might make it more difficult for users to find what they are interested in: the changes they need to do to their app after they update.
IMO
is only of interest for power users - they would know how to find the PR (from the changelog) and check the description and/or the comments in the code.
That being said, one thing I should have mentioned is how this issue manifest (i.e. the generated error).
That's my 2 cents but feel free to add an item to the weekly agenda if you think we should discuss more about that.