feat: support empty fallbacks #146
Merged
Conversation
This file contains 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
markdalgleish
commented
Aug 18, 2023
| @@ -10,6 +10,7 @@ import type esbuild from 'esbuild'; | |||
| const NAME = 'node-modules-polyfills'; | |||
|
|
|||
| export interface NodePolyfillsOptions { | |||
| fallback?: 'empty' | 'none'; | |||
There was a problem hiding this comment.
I went with this API so that it's more future proof, as opposed to emptyFallback: true or something similar. This way we can introduce different fallback behaviour in the future if needed without it being a breaking change, e.g. fallback: 'error' or fallback: (args) => { /* some custom logic... */ }. It also nicely mirrors the modules config option, making it clear that it's providing the same result.
imranbarbhuiya
approved these changes
Aug 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The need for this came up in the context of Remix. Since route files in a Remix app contain code for both server and client and go through separate esbuild pipelines, we need to allow imports from all Node builtins in the client build, even those that don't have any polyfills. We then rely on tree shaking to remove code from unused imports in the client bundle.
Right now we handle unpolyfilled Node builtins by marking them as
externalin our esbuild config, but this approach is causing other issues (remix-run/remix#5521). It also means that any unpolyfilled Node builtin imports are left in the client bundle and throw an error at runtime. This change would allow us to remove ourexternalsconfig and fix this issue.Status and versioning classification: