-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Scripts: Transform scripts directory to type:module #24698
Conversation
f428363
to
760f2b8
Compare
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: typescript@5.1.6 |
760f2b8
to
6bbedf1
Compare
6bbedf1
to
b7ef8c5
Compare
8286a8d
to
85fe759
Compare
combine-compodoc.ts
What do you mean by this?
Our bundle scripts still use tsup, I don't see any changes to that? (Which I'm happy about, because tsup has many advantages over rawdogging esbuild for us, mainly that it supports generating |
Failed to publish canary version of this pull request, triggered by @valentinpalkovic. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/6795743645 |
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.
Awesome cleanup here!
}; | ||
}) | ||
.reduce((acc, next) => { | ||
acc[next.name] = next; | ||
return acc; | ||
}, {}); | ||
}, {} as Record<string, { name: string; defaultValue: boolean; suffix: string; helpText: string }>); |
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.
Instead of the cast, you might be able to provide a generic to reduce
instead, which is a bit safer normally.
@ndelangen, I will wait for your approval before this gets merged to get in your opinion. |
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.
💪🧹
@ndelangen friendly reminder |
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
esbuild-register from scripts/node_modules
"check": "node --loader esbuild-register/loader -r esbuild-register ../../../scripts/prepare/check.ts", | ||
"prep": "node --loader esbuild-register/loader -r esbuild-register ../../../scripts/prepare/esm-bundle.ts" | ||
"check": "node --loader ../../../scripts/node_modules/esbuild-register/loader.js -r ../../../scripts/node_modules/esbuild-register/register.js ../../../scripts/prepare/check.ts", | ||
"prep": "node --loader ../../../scripts/node_modules/esbuild-register/loader.js -r ../../../scripts/node_modules/esbuild-register/register.js ../../../scripts/prepare/esm-bundle.ts" |
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.
This is pretty dang ugly. Are you sure this isn't a good enough reason to explore using tsx
instead? ;)
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.
https://www.npmjs.com/package/tsx
@valentinpalkovic let's try this tomorrow.
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.
tsx
will be installed in scripts
. It would then look like this:
"check": "../../../scripts/node_modules/tsx/index.js ../../../scripts/prepare/check.ts"
This is still ugly. A bit less ugly than the current solution, but still ugly. I don't see strong reasons for now to invest even more time into this PR.
Why should we introduce a tool, which builds on top of esbuild, if we can use esbuild directly and have no issues with it? I haven't seen strong reasons and arguments for introducing it so far.
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.
oh yeah that's not much better. Seems like something that can be cleaned up later if we find a nice way. Using NODE_PATH
could also be an option, though I feel like it's discouraged / deprecated these days.
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.
Hey @IanVS
I have spiked a bit a solution using tsx
. I haven't invested much time, but the following occurred:
The strange thing is, that named exports are not allowed to be imported with named imports.
e.g. fs-extra
Currently, we do named imports like this:
import { readJson } from 'fs-extra';
Node then complains and says:
import { readFile } from 'fs-extra';
^
SyntaxError: The requested module 'fs-extra' does not provide an export named 'readFile'
at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)
Indeed, if I go to the Readme of fs-extra the following is written:
ESM
There is also an fs-extra/esm import, that supports both default and named exports. However, note that fs methods are not included in fs-extra/esm; you still need to import fs and/or fs/promises separately:import { readFileSync } from 'fs' import { readFile } from 'fs/promises' import { outputFile, outputFileSync } from 'fs-extra/esm'Default exports are supported:
import fs from 'fs' import fse from 'fs-extra/esm' // fse.readFileSync is not a function; must use fs.readFileSync
So, I have to change all named imports to default imports.
The same error then occurs on relative file imports, like this one:
import { allTemplates } from '../code/lib/cli/src/sandbox-templates';
/Users/valentinpalkovic/Projects/storybook-next/scripts/task.ts:33
allTemplates as TEMPLATES,
^
SyntaxError: The requested module '../code/lib/cli/src/sandbox-templates' does not provide an export named 'allTemplates'
at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)
I am not sure what’s going on here or why the error occurs with tsx, but not with the esbuild-loader
Closes N/A
What I did
"type": "module"
inscripts/package.json
scripts
byesbuild
toolchainscripts/check-package.js
require.main === module
expression by esm equivalentscripts/jest.config.cjs
to tell Jest how to properly run tests in esm environmentscripts/tsconfig.json
to tell Typescript, that we are using ESM, but a bundler (moduleResolution setting) is used in the end. With that, we are not forced to use extensions for relative imports.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:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts
Make 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 pull request has been released as version
0.0.0-pr-24698-sha-758c0de1
. Install it by pinning all your Storybook dependencies to that version.More information
0.0.0-pr-24698-sha-758c0de1
valentin/use-type-module-in-scripts
758c0de1
1699434116
)To request a new release of this pull request, mention the
@storybookjs/core
team.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=24698