fix: dynamically resolve Turbopack external module mappings#1139
fix: dynamically resolve Turbopack external module mappings#1139james-elicx merged 15 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: a75fac1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
james-elicx
force-pushed
the
james/turbopack-externals
branch
from
e91953c to
f2932bd
Compare
james-elicx
force-pushed
the
james/turbopack-externals
branch
from
f2932bd to
ea1d8ff
Compare
james-elicx
force-pushed
the
james/turbopack-externals
branch
from
a7f2cec to
b98c6e1
Compare
|
Thanks for the PR James! When I implemented support for OG, I knew it was brittle and would probably need to be handled in a better way. I'll take a look later this week. |
conico974
left a comment
conico974
left a comment
There was a problem hiding this comment.
LGTM but I think the test is in the wrong place
| * Discover Turbopack external module mappings by reading symlinks in .next/node_modules/. | ||
| * | ||
| * Turbopack externalizes packages listed in serverExternalPackages and creates hashed | ||
| * identifiers (e.g. "shiki-43d062b67f27bbdc") with symlinks in .next/node_modules/ pointing |
There was a problem hiding this comment.
Thought while reading this JSDoc: can this be implemented as an ESBuild plugin?
i.e. when we try to resolve shiki-43d062b67f27bbdc, we would replace with the actual path
There was a problem hiding this comment.
I have a feeling it might not solve all the cases where this can happen with dynamic imports
james-elicx
changed the title
|
is it planned to be merged? thx. |
| * statically analyze those hashed names. This function discovers the mappings so we can | ||
| * generate explicit switch cases for the bundler. | ||
| * | ||
| * @param filePath Absolute path to the Turbopack runtime file (e.g. `.next/server/chunks/ssr/[turbopack]_runtime.js`), |
There was a problem hiding this comment.
.next/server/chunks/ssr/[turbopack]_runtime.js does not look like an absolute path. Could we pass the build options instead?
There was a problem hiding this comment.
It's an absolute path
https://github.com/opennextjs/opennextjs-aws/blob/4bdfd292f985913f3298eb3f30a7790b877e0464/packages/open-next/src/build/copyTracedFiles.ts#L151
https://github.com/opennextjs/opennextjs-aws/blob/4bdfd292f985913f3298eb3f30a7790b877e0464/packages/open-next/src/build/copyTracedFiles.ts#L221
| // E.g. for hashedName "shiki-43d062b67f27bbdc", this matches strings like | ||
| // "shiki-43d062b67f27bbdc/wasm" or "shiki-43d062b67f27bbdc/engine/javascript". | ||
| // The hashedName is escaped to safely use it as a literal in the regex pattern. | ||
| const escaped = hashedName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); |
There was a problem hiding this comment.
nit: don't we have an util for that? (IIRC it's coming to latest v8)
There was a problem hiding this comment.
I believe we still support Node v22, and it looks like RegExp.escape(...) is only available from v24 onwards according to mdn
There was a problem hiding this comment.
Iirc we have a util we use for i.e. path escaping (in aws)?
There was a problem hiding this comment.
Oh sorry I thought you meant a runtime util when I read V8. My bad. There's the cross platform path one that handles escaping.
|
Thanks for the updates James, I'll review tomorrow |
4 participants
Disclaimer that this was 100% debugged and patched by Opus 4.6 agents in a different project that I was trying to get running on Workers. I've moved the patch back into this repo and added a regression test.
Some packages that are externalised by turbopack fail to be loaded/resolved on workerd, and esbuild can't statically analyse them properly when bundling from what I understand.
This PR does the following:
Without this patch: