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: transpile minimum node_modules #10725

Merged
merged 10 commits into from
May 18, 2020
Merged

Conversation

tooppaaa
Copy link
Contributor

Issue: Famous IE 11 #10674 #10670 #10486

What I did

Transpile minimum node_modules.

This is a slighlty different approach than #10683.

How to test

  • IE 11

@@ -25,7 +25,16 @@ const projectRoot = () => {

export const includePaths = [projectRoot()];
export const nodeModulesPaths = path.resolve('./node_modules');
export const excludePaths = [/node_modules/];

// Run `npx are-you-es5 check lib/core -r`
Copy link
Member

Choose a reason for hiding this comment

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

How long does it take to run this command, could we potentially run it before running storybook on the user's machine?

Copy link
Contributor Author

@tooppaaa tooppaaa May 11, 2020

Choose a reason for hiding this comment

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

It's pretty quick but it's only CLI and the output is as follow

$ npx are-you-es5 check lib/core -r
npx : 4 installé(s) en 2.09s
❌ @storybook/node-logger is not ES5
⚠️ @types/node-fetch was not checked because no entry script
was found
❌ better-opn is not ES5
❌ boxen is not ES5
❌ chalk is not ES5
❌ commander is not ES5
❌ find-cache-dir is not ES5
❌ find-up is not ES5
❌ fs-extra is not ES5
❌ json5 is not ES5
❌ node-fetch is not ES5
❌ pkg-dir is not ES5
⚠️ react-dev-utils was not checked because no entry script w
as found
❌ resolve-from is not ES5
❌ semver is not ES5

Babel-loader exclude regex:
/[\/]node_modules[\/] (?!(@storybook/node-logger|better-opn|boxen|chalk|commander|find-cache-dir|find-up|fs-extra|json5|node-fetch|pkg-dir|resolve-from|semver)[\/])/

Copy link
Member

Choose a reason for hiding this comment

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

if it's expensive, we should make it easier to update this code using a script?

// Run `npx are-you-es5 check lib/core -r`
// to get this regexp
export const excludePaths = [
/[\\/]node_modules[\\/](?!(@storybook\/node-logger|are-you-es5|better-opn|boxen|chalk|commander|find-cache-dir|find-up|fs-extra|json5|node-fetch|pkg-dir|resolve-from|semver)[\\/])/,
Copy link
Member

Choose a reason for hiding this comment

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

why is @storybook/node-logger in this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I unfortunately don't know. I was surprised as well but also didn't wanted to alter the output of the regexp

Copy link
Member

Choose a reason for hiding this comment

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

Because it only is run in node, and so ES6 is fine?

Copy link
Member

Choose a reason for hiding this comment

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

it should not be bundled into client-code ever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

being in this regexp doesn't mean you'll get bundled. It's only "If you're being bundled and going though babel, you'll get transpiled"

Copy link
Member

Choose a reason for hiding this comment

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

of course, the are-you-es5 just detects the code being ES6 and adds it to the list.

@tooppaaa
Copy link
Contributor Author

Can't reproduce CLI error locally, can someone try on a mac ? :(

lib/core/src/server/common/babel.js Outdated Show resolved Hide resolved
/[\\/]node_modules[\\/](?!(@storybook\/node-logger|are-you-es5|better-opn|boxen|chalk|commander|find-cache-dir|find-up|fs-extra|json5|node-fetch|pkg-dir|resolve-from|semver)[\\/])/,
// Never transpile again the following:
/[\\/]node_modules[\\/]webpack/,
/[\\/]node_modules[\\/]core-js/,
Copy link
Member

Choose a reason for hiding this comment

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

@mrmckeb @gaetanmaisse what does this mean for yarn 2?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing the include will not match anything in yarn2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @gaetanmaisse once said:

it will not be worse than what we have today

],
require.resolve('babel-plugin-macros'),
...plugins,
// Shoud only be done on manager to to ember
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain? why not always?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
I don't know much about ember, just found that it is fine without that transform.
It can't be in the common plugins

Copy link
Member

Choose a reason for hiding this comment

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

ok

lib/core/src/server/preview/iframe-webpack.config.js Outdated Show resolved Hide resolved
@ndelangen ndelangen merged commit 4b7c1a4 into next May 18, 2020
@ndelangen ndelangen deleted the ie11_again_andAgain_theReturn branch May 18, 2020 12:11
gaetanmaisse added a commit that referenced this pull request May 18, 2020
All theses deps have been used in `@storybook/core` in #10725
But haven't been properly added as dependencies.
gaetanmaisse added a commit that referenced this pull request May 18, 2020
All theses deps have been used in `@storybook/core` in #10725
But haven't been properly added as dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants