Skip to content
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

Build: Replace esbuild-register by jiti #28796

Merged
merged 11 commits into from
Aug 7, 2024
Merged

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Aug 3, 2024

Replaces: #28046

What I did

Since the migration to bun seems to encounter issues and esbuild-register is not working with new node versions, this PR propes to migrate to jiti for the ts executation during development.

Only a few very minimal changes were necessary to execute the scripts with jiti.

Moreover, I've added bun as a dev dependency of the scripts package so that one doesn't need to install it separately.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

Run yarn start as suggested in the contribution guidelines.

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team 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>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 76.3 MB 76.3 MB 0 B 0.74 0%
initSize 171 MB 171 MB -5.63 kB -0.81 0%
diffSize 95 MB 95 MB -5.63 kB -0.81 0%
buildSize 7.42 MB 7.42 MB 0 B -1.36 0%
buildSbAddonsSize 1.61 MB 1.61 MB 0 B -1.35 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.29 MB 2.29 MB 0 B -0.73 0%
buildSbPreviewSize 351 kB 351 kB 0 B 0.23 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.45 MB 4.45 MB 0 B -1.28 0%
buildPreviewSize 2.97 MB 2.97 MB 0 B -1.36 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 12.1s 23.4s 11.2s 1.16 48.1%
generateTime 22.1s 22s -132ms -0.1 -0.6%
initTime 19.7s 19.5s -133ms -0.58 -0.7%
buildTime 11.9s 14s 2.1s 0.27 15.2%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 7.5s 8.1s 562ms -0.26 6.9%
devManagerResponsive 4.9s 5s 93ms -0.58 1.8%
devManagerHeaderVisible 722ms 887ms 165ms 0.13 18.6%
devManagerIndexVisible 755ms 923ms 168ms 0.13 18.2%
devStoryVisibleUncached 929ms 1.4s 563ms 1.3 🔺37.7%
devStoryVisible 785ms 948ms 163ms 0.11 17.2%
devAutodocsVisible 674ms 746ms 72ms -0.1 9.7%
devMDXVisible 616ms 705ms 89ms -0.35 12.6%
buildManagerHeaderVisible 667ms 794ms 127ms -0.02 16%
buildManagerIndexVisible 668ms 801ms 133ms -0.03 16.6%
buildStoryVisible 708ms 843ms 135ms -0.08 16%
buildAutodocsVisible 661ms 720ms 59ms -0.17 8.2%
buildMDXVisible 578ms 715ms 137ms 0.2 19.2%

Greptile Summary

This pull request replaces esbuild-register with jiti for TypeScript execution during development to address compatibility issues with newer Node.js versions.

  • Updated .circleci/config.yml to use jiti instead of esbuild-register.
  • Modified package.json files across multiple addons and frameworks to replace esbuild-register with jiti in check and prep scripts.
  • Added bun as a dev dependency in several packages to streamline the development setup.
  • Ensured minimal changes to maintain existing functionality while improving compatibility and development workflow.

Copy link

nx-cloud bot commented Aug 3, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 44ee488. 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 target

Sent with 💌 from NxCloud.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

30 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@ndelangen
Copy link
Member

Appreciate the PR @tobiasdiez

The Ci is failing however.

@tobiasdiez
Copy link
Contributor Author

The error comes from tempy (via temp-dir):

/tmp/storybook/scripts/node_modules/temp-dir/index.js:4
const temporaryDirectory = await _nodeFs.promises.realpath(_nodeOs.default.tmpdir());var _default = exports.default =
^^^^^
SyntaxError: await is only valid in async functions and the top level bodies of module

Jiti currently does not provide support for top level await (but it's next version will): unjs/jiti#72
Would you be okay with replacing the temporary directory creation with node's fs.mkdtemp?

@ndelangen
Copy link
Member

ndelangen commented Aug 7, 2024

Yeah, I'd welcome a replacement.

I'll help out, as it would be amazing to get rid of esbuild-register for our builds.

@ndelangen ndelangen changed the title Build: replace esbuild-register by jiti Build: Replace esbuild-register by jiti Aug 7, 2024
@ndelangen ndelangen added the build Internal-facing build tooling & test updates label Aug 7, 2024
@ndelangen ndelangen added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:normal labels Aug 7, 2024
@ndelangen ndelangen merged commit 1a6bb99 into storybookjs:next Aug 7, 2024
4 of 5 checks passed
@github-actions github-actions bot mentioned this pull request Aug 7, 2024
29 tasks
@ndelangen ndelangen mentioned this pull request Aug 7, 2024
8 tasks
@tobiasdiez tobiasdiez deleted the jiti branch August 7, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates ci:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants