storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.82k stars 9.34k forks source link

is-map: 'return' outside of function #9154

Closed hckhanh closed 4 years ago

hckhanh commented 4 years ago

When I try to update to v5.3.0-beta.23 I get this error when building the project

ERROR in ./node_modules/is-map/index.js
Module build failed (from ./node_modules/babel-loader/lib/index.js):
SyntaxError: node_modules\is-map\index.js: 'return' outside of function (12:1)
TFisch commented 4 years ago

still seeing the error after npm i --save-dev --save-exact airbnb-js-shims@2.2.0 explicit. This work around is for current version of SB? Or do I need to rollback? my current SB dependencies are,

"@storybook/addon-actions": "^5.0.11",
"@storybook/addon-links": "^5.0.11",
"@storybook/addons": "^5.0.11",
"@storybook/react": "^5.0.11",
"airbnb-js-shims": "2.2.0",
"storybook-react-router": "^1.0.5",
thuringia commented 4 years ago

@TFisch I had to add

"resolutions": {
    "airbnb-js-shims": "2.2.0",
    "es-get-iterator": "1.0.1",
    "is-map": "1.0.1",
    "is-set": "1.0.0"
  },

to package.json and regenerate the lockfile

~~Update: This doesn't appear to be a great workaround. Storybook builds now, but creates a very long list of warnings with imports that cannot be found. I get the Storybook UI now up and running, but none of my stories are present...~~ There was a weird exclusion rule for node_modules left from the Gatsby starter. Everything is working now

ljharb commented 4 years ago

@sahariko neither package is faulty. Top level return is valid in node modules; it’s probably webpack not being configured properly.

TFisch commented 4 years ago

@thuringia thanks for the suggestion but I'm using npm so resolutions seems to not be accessible.

sahariko commented 4 years ago

@ljharb Regardless, I think it's a good idea to provide support for consumers that use webpack, which is arguably a whole lot.

ljharb commented 4 years ago

That is quite true, but webpack v4.31+ supports top level return as far as i am aware.

thuringia commented 4 years ago

@TFisch did you try to add is-set and is-map as dependencies and dedupe? That should create the same effect using npm

ljharb commented 4 years ago

If someone can identify the exact thing that’s failing to properly parse is-map and is-set, i could easily be convinced to alter those packages to not rely on that feature of CJS. Has anyone done that?

sahariko commented 4 years ago

Are you inherently opposed to this fix - https://github.com/inspect-js/is-map/pull/5? It also cleans up the module a little bit.

ljharb commented 4 years ago

I’ll review it when i get to a computer, but I’m philosophically opposed to any change forced on my module by broken tools, at least not without understanding exactly which tools are broken so i can pursue fixing them.

TFisch commented 4 years ago

@thuringia so adding "es-get-iterator": "1.0.1", "is-map": "1.0.1", "is-set": "1.0.0" as dependencies, then "airbnb-js-shims": "2.2.0", as a dev-depend? I've done that followed by npm dedupe and I'm still seeing the error.

Update: Im now receiving the same error, but its pointing to the modules of the dependency instead of the project.

Previous error path: myProject/node_modules/is-map/index.js

New error path: myProject/node_modules/es-get-iterator/node_modules/is-map/index.js

Did I do something silly?

eric-burel commented 4 years ago

That is quite true, but webpack v4.31+ supports top level return as far as i am aware.

It also brings some bugs, we had to lock to 4.28 due to this issue with dynamic-loader: https://github.com/webpack/webpack/issues/8656.

Maybe it has been solved since but it was a real blocker for a Webpack update

eric-burel commented 4 years ago

It seems that some other package may import es-get-iterator, my own workaround does not work this good :/ what's your versions of Webpack for those having solved the issue or still facing it?

eric-burel commented 4 years ago

Maybe try npm i --save-dev --save-exact airbnb-js-shims@2.2.0 promise.allsettled@1.0.1

Basically subdependencies don't necessarily lock version so we have to identify all culprits for this to work, so we have 0 dependency on is-map, is-set.

The real solution is probably either including "faulty" modules in the Babel build, or updating Webpack to support those modules. Basically the is-set, is-map modules are "right" but many build system may lag behind for some reason (regression in new versions of Webpack etc.)

Edit: tested, working for me

emoriarty commented 4 years ago

I got the workaround working following what @eric-burel and @thuringia mentioned. Thanks, guys!

What I've done is installing the conflicting libs to a previous version.

yarn add is-set@1.0.0 is-map@1.0.1 es-get-iterator@1.0.1 airbnb-js-shims@2.2.0

Once installed, then added the resolutions property in package.json.

  "resolutions": {
    "airbnb-js-shims": "2.2.0",
    "es-get-iterator": "1.0.1",
    "is-map": "1.0.1",
    "is-set": "1.0.0"
  },

For those using npm, perhaps you can check npm-force-resolutions.

ljharb commented 4 years ago

Essentially, the issue is caused both by webpack being broken prior to v4.31, and babel/CRA not enabling allowTopLevelReturn by default, and it's worsened by the (highly unsafe) behavior of transpiling third party code.

I'll have a change published for is-map and is-set soon which should address this.

ljharb commented 4 years ago

v2.0.1 of both is-map and is-set have been released; this can be closed.

shilman commented 4 years ago

Thanks so much for the fix @ljharb!

It sounds like there's something fishy going on in the ecosystem, and we'll do our best to sort this out on the Storybook side too.

In the meantime, hopefully this unbreaks a lot of people! 🙌

vijayakumartech commented 4 years ago

With Jenkins build throwing the same error.