Enable Babel syntax support for the dynamic import specification within node_modules such that webpack can handle the statements

[stereobooster]

Is this a bug report?

Yes

Did you try recovering your dependencies?

(Write your answer here.)

yarn -v
1.5.1

Which terms did you search for in User Guide?

experimental syntax 'dynamicImport'

Environment

npx create-react-app --info .
Please specify the project directory:
  create-react-app <project-directory>

But this is what is relevant:

node -v
v8.6.0
yarn -v
1.5.1
"react-scripts": "2.0.0-next.b2fd8db8",
"react-styleguidist": "7.0.14"
MacOS X

Steps to Reproduce

1. git clone https://github.com/stereobooster/async-precious.git
2. cd async-precious
3. git checkout styleguidist-bug
4. yarn
5. yarn styleguide

Expected Behavior

No error

Actual Behavior

styleguidist server
./node_modules/react-styleguidist/lib/rsg-components/Editor/EditorLoader.js
Syntax error: async-precious/node_modules/react-styleguidist/lib/rsg-components/Editor/EditorLoader.js: Support for the experimental syntax 'dynamicImport' isn't currently enabled (36:4):

  34 | 			var _this2 = this;
  35 |
> 36 | 			import('rsg-components/Editor/Editor').then(function (module) {
     | 			^
  37 | 				_this2.setState({ editor: module.default });
  38 | 			});
  39 | 		}

Add @babel/plugin-syntax-dynamic-import (https://git.io/vb4Sv) to the 'plugins' section of your Babel config to enable parsing.
	from thread-loader (worker 0)

Reproducible Demo

1. git clone https://github.com/stereobooster/async-precious.git
2. cd async-precious
3. git checkout styleguidist-bug

As far as I understand this happens due to #3776. Related issue styleguidist/react-styleguidist#987

[jmlivingston]

I'm not the maintainer, but can reproduce the bug above.

You can also simply make a reference to the component and just start create-react-app.

1. Add following added to App.js

import EditorLoader from '../node_modules/react-styleguidist/lib/rsg-components/Editor/EditorLoader'

2. Start create-react-app yarn start

[ridem]

I can confirm I experience the same issue, but I'm not sure on which side (CRA vs Styleguidist) it falls.

styleguidist is a dependency, and as such is going through the dependencies.js preset instead of the index.js one.

Everything that's not in the app path (./src/*) is considered as a dependency, and is compiled without the fancy ES syntax/features (https://github.com/facebook/create-react-app/blob/next/packages/react-scripts/config/webpack.config.dev.js#L243)

From what I understand of the issue, either:

• Styleguidist is not willing to get rid of their one dynamic import and it won't be supported anymore by CRA, unless CRA adds some whitelist of very specific node_modules paths of the plugins they want to support through advanced ES syntax/features, or adds @babel/plugin-syntax-dynamic-import to the dependencies preset plugin (could be the easiest option)
• It's a bug on the styleguidist side, as they're using an ES syntax that is "only" at stage 3 in the code of a public module

Note: I can confirm that the dynamic import syntax issue is the only thing preventing styleguidist from running properly. For example, manually adding @babel/plugin-syntax-dynamic-import to the list of plugins in the dependencies.js file fixes the issue

[stereobooster]

It's a bug on the styleguidist side, as they're using an ES feature that is "only" at stage 3 in the code of a public module

But create-react-app itself supports dynamic import, that is how react-loadable works in c-r-a.

[ridem]

But create-react-app itself supports dynamic import, that is how react-loadable works in c-r-a.

I guess the whole point of why react-loadable works is precisely that they don't use the stage 3 ES dynamic imports in their own codebase

[stereobooster]

from react-loadable homepage

const LoadableComponent = Loadable({
  loader: () => import('./my-component'), // <-- dynamic import
  loading: Loading,
});

[ridem]

Yes, it's in their homepage, as an example of a code that you would put in your own app, so that's fine. I meant that they are not using the dynamic import syntax in their own codebase, and that's probably why it stills works with CRA@next

[stereobooster]

c-r-a itself supports dynamic import, I can use dynamic import in my app code (based on c-r-a). Why other projects can not use it? This doesn't make sense to me

[ridem]

I too can use dynamic imports in my CRA app code. Starting from the next version of CRA, non-basic ES in node_modules isn't allowed (see my comment with the links to the codebase that does that). You can still use most of the advanced ES6 syntax/features in your app code.

[Timer]

We only compile stage 4 features that are used in your dependencies; there is no guarantee a stage 3 will advance to stage 4 without changes.

This is by design. Compiling away experimental specifications is a slippery slope to causing churn in the community by supporting features which may never make it into the language and then break backwards compatibility, preventing future advancement.

[stereobooster]

@Timer this totally makes sense, but what is the resolution here? Should we reopen ticket in styleguidist? Or can I opt-out node modules compilation for this package?

[Timer]

I'm not sure of the best resolution here -- we received significant flak about not compiling node_modules, so if this ends up breaking packages who use experimental syntax only understood by a bundler [webpack], I think I can live with that.

This case might be semi-unique because it uses the import() specification, which we suggest for usage -- I wouldn't see major harm in allowing this to be parsed so that webpack can handle it.

Where I'm torn is that if the import() specification changes, we are willing to provide codemods to upgrade a create-react-app codebase, though updating packages may be more difficult. However, since this is already widely adopted in the community (by webpack users), I think the potential for change here is minimal.

I'd like to hear what other maintainers think on the subject, @iansu @gaearon.

[Timer]

@stereobooster I'd reopen the issue on their end for now; it'd be nice if they could drop usage of import().

They seem to do some really fishy stuff which may break at any time, it's discouraging to see things like this (I'm not sure if this stuff is imported by usage with create-react-app or not, but it looks like it based on the rule they're disabling):
https://github.com/styleguidist/react-styleguidist/blob/f5b0057eee4c171d695d6d2d766f318c74b9c608/src/index.js#L19-L20

[sapegin]

Let's not call some code “fishy” only because you don’t know what it does and constraints we have. It also has nothing to do with the import issue.

What would you suggest to use instead of the import? I'm happy to replace it with a better solution. We used to use System.import but webpack 4 shows a deprecation warning about it.

[bjankord]

Thanks @ridem for detailing the issue. I've been trying to figure out why my package using dynamic imports wasn't working when it was used with the CRA v2 branch. We currently use require.ensure to create code split points for translation / intl data allowing us to only load the translation / intl data on demand per locale. I saw that require.ensure is blacklisted in CRA, so I switched the code over to use dynamic imports but kept getting a vague compilation error using CRA: "Failed to compile" and then the file path to the file using dynamic-imports.

Then I saw this issue and reading through it, learned that CRA's support for dynamic imports is only for "app" code and does not support code imported from node_modules. As @Timer mentioned, "We only compile stage 4 features that are used in your dependencies;" and I think that makes sense.

I would encourage the CRA team to reconsider this stance in regards to adding the babel plugin-syntax-dynamic-import to dependencies.js or note on the README section for dynamic imports that they are only supported in "app" code.

[satya164]

@sapegin have you tried if require.ensure works with webpack 4? certainly not better solution and webpack specific (shouldn't be a problem in your case I assume), but I might work as an alternative.

edit: sorry didn't see @bjankord's comment

[bjankord]

Looks like this is resolved via this commit: e8b0ee8#diff-e4eb38a3161bed144100754a3e97763d

[stereobooster]

seems to be solved, but haven't tested it. Is there alpha release with this commit?

[Timer]

Yes @stereobooster, see next tag