ESM: Replace esbuild-register with jiti in core
#29196
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
ndelangen
added
ci: do not merge
maintenance
|
What error do you see? Jiti should be able to handle cjs > esm imports. You might need to play around with the |
|
@tobiasdiez I was seeing 2 issues. 1: We have a 2: I had trouble getting jiti v2 to work for our tool chain to compile. This problem I spend less time on investigating, so I don't have a lot of details on this one. I left a comment to should help you reproduce the error I ran into if you have the appetite. If you do investigate and find something we are doing wrong, please let me know 🙏 |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit f05f8e2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
To give a real life example, a user might have a This is written in ESM, but it's in a project that does not have With our current approach with I'd argue that Jiti is doing the right thing here, but it doesn't change the fact that this would break a lot of users - especially because we generate a default |
|
Strange, I thought in such an ambiguous case (js extension, no type in package.json) jiti would check for esm-like syntax in the code and then transpile it to cjs. What is the debug output in this case? |
|
These are the relevant parts from the log with debug enabled: The only thing that stands out to me is that here's the full log: You can experience this yourself @tobiasdiez by cloning the branch and reverting the changes in this line - which was done to mitigate the issue. https://github.com/storybookjs/storybook/pull/29196/files#diff-71fb14d7214019004e69757298d9055859d208da11f28dbc363eacb5d672d0ceR44 The issue is happening because the preset in // src/plugins/docgen-handlers/actualNameHandler.ts
var import_react_docgen, getNameOrValue, isReactForwardRefCall, actualNameHandler, actualNameHandler_default;
var init_actualNameHandler = __esm({
"src/plugins/docgen-handlers/actualNameHandler.ts"() {
"use strict";
import_react_docgen = require("react-docgen");
({ getNameOrValue, isReactForwardRefCall } = import_react_docgen.utils);But While we could solve this instance internally (and we have), but this would only be one of many such cases. And we expect that this would break for users/addons too pretty easily in similar scenarios. It's entirely possible we've missed something glaringly obvious 🙏 |
|
So that's the problem
that @ndelangen mentions above (#29196 (comment)), right? I don't think that there is much here that you can do. If someone ships a What may also help is to use |
Closes #29082
What I did
I replaced
esbuild-registerwithjiti.We use it to be able to
require().ts-files (such asmain.ts) during the loading of presets.Jitihas a different approach to enabling loading.ts-files thanesbuild-register.Jitiseems to transpile the files individually, and skip files that don't need to be transpiled.esbuild-registeractually bundles the entrypoint into a single (in memory?) file.The difference between these approaches is that
esbuild-registersolves any incorrect CJS > EMS usage by bundling;jitiwill leave those problematic connections in place, which then fail at runtime.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>