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

Core: Disable esbuild on files imported from node_modules #23018

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

tmeasday
Copy link
Member

Running esbuild over some of our larger imports (e.g. typescript, imported by react-docgen-typescript-plugin) can significantly increase their import time (not totally clear why as they are JS files).

I'm not sure why we changed this setting from the default (to not run esbuild over node_modules) in the first place.

What I did

Switched back to the default -- to leave files from node_modules untranspiled.

How to test

Hopefully should be covered by the daily sandboxes. But it would be very interesting to see how this affects our measured timings.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@tmeasday tmeasday added performance issue ci:daily Run the CI jobs that normally run in the daily job. labels Jun 10, 2023
@tmeasday tmeasday requested review from shilman and ndelangen June 10, 2023 00:00
@shilman shilman changed the title Performance: Switch to not running esbuild on files imported from node_modules. Core: Disable esbuild on files imported from node_modules Jun 10, 2023
@shilman shilman added core maintenance User-facing maintenance tasks labels Jun 10, 2023
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This looks OK to me, but tbh I really don't understand the implications. I guess this will have problems with ESM-only packages until node supports ESM out of the box?

@shilman shilman removed the ci:daily Run the CI jobs that normally run in the daily job. label Jun 11, 2023
@ndelangen
Copy link
Member

Node does support ESM out of the box starting node16 (which is the minimum for storybook already), as long as you import ESM with an imperative import. You can't require an ESM.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

If we need to, we can add this as a config option to main.js later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants