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

0.5.13 Multiple default exports crashing meteor and babel minifier #74

Closed
amsully opened this issue Jul 23, 2019 · 16 comments
Closed

0.5.13 Multiple default exports crashing meteor and babel minifier #74

amsully opened this issue Jul 23, 2019 · 16 comments

Comments

@amsully
Copy link

amsully commented Jul 23, 2019

Describe the bug

Error within UI after successful build:
Screen Shot 2019-07-23 at 11 17 57 AM
Screen Shot 2019-07-23 at 11 17 46 AM

Error thrown during babel minification process in meteor:

 
export default Component; 
 
at maybeThrowMinifyErrorBySourceFile 
(packages/minifyStdJS/plugin/minify-js.js:96:26) 
at files.forEach.file (packages/minifyStdJS/plugin/minify-js.js:135:9) 
at Array.forEach (<anonymous>) 
at MeteorBabelMinifier.processFilesForBundle 
(packages/minifyStdJS/plugin/minify-js.js:118:9) 
 
 
While minifying app code: 
packages/minifyStdJS/plugin/minify-js.js:96:26: Babili minification error 
within packages/modules.js: 
node_modules/react-spinners/dist/BeatLoader.js 
 
Only one default export allowed per module.: 
 
export default Component; 
 
at maybeThrowMinifyErrorBySourceFile 
(packages/minifyStdJS/plugin/minify-js.js:96:26) 
at files.forEach.file (packages/minifyStdJS/plugin/minify-js.js:135:9) 
at Array.forEach (<anonymous>) 
at MeteorBabelMinifier.processFilesForBundle 
(packages/minifyStdJS/plugin/minify-js.js:118:9)

To Reproduce
Steps to reproduce the behavior:

  1. Meteor app using version 1.8.1. Babel runtime 7.3.4.
  2. Import react-spinner 0.5.13 into app and throws errors shown in screenshot.

Temporary Solution
Our solution was to downgrade to version 0.5.12 until this is resolved.

@davidhu2000
Copy link
Owner

hmm, interesting. 0.5.13 only have readme and changelog updates. It was a patch to fix the props table in readme.

Just want to confirm, downgrading to 0.5.12 fixes your issue completely?

@alexmarmon
Copy link

@davidhu2000 I just came across this as well. Downgrading to 0.5.12 completely fixed for me.

@amsully
Copy link
Author

amsully commented Jul 23, 2019

Yes downgrading fixes the issue completely, our diff for the fix:

-    "react-spinners": "^0.5.4",
+    "react-spinners": "0.5.12",

^0.5.4 was defaulting to 0.5.13 when building from scratch. Locally I have switched back and forth between 0.5.12 and 0.5.13 and replicated the problem. Only 0.5.13 has a problem.

I also get the same issue with 0.6.0-alpha.1.

@openjck
Copy link

openjck commented Jul 23, 2019

I'm seeing a related issue on ensemble when running jest tests while react-spinners@0.5.13 is installed:

> CI=true react-scripts test

FAIL src/tests/jest/MetricOverview.test.js
  ● Test suite failed to run

    [...]/ensemble/node_modules/react-spinners/dist/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import BarLoaderComponent from "./BarLoader";
                                                                                                    ^^^^^^^^^^^^^^^^^^

    SyntaxError: Unexpected identifier

      1 | import React from 'react';
    > 2 | import { PulseLoader } from 'react-spinners';
        | ^
      3 | 
      4 | import './css/Spinner.css';
      5 | 

      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:471:17)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:513:25)
      at Object.<anonymous> (src/components/views/Spinner.js:2:1)

FAIL src/tests/jest/Dashboard.test.js
  ● Test suite failed to run

    [...]/ensemble/node_modules/react-spinners/dist/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import BarLoaderComponent from "./BarLoader";
                                                                                                    ^^^^^^^^^^^^^^^^^^

    SyntaxError: Unexpected identifier

      1 | import React from 'react';
    > 2 | import { PulseLoader } from 'react-spinners';
        | ^
      3 | 
      4 | import './css/Spinner.css';
      5 | 

      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:471:17)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:513:25)
      at Object.<anonymous> (src/components/views/Spinner.js:2:1)

Downgrading to 0.5.12 fixes it.

@814k31
Copy link

814k31 commented Jul 26, 2019

0.5.13 breaks my tests as well, downgrading to 12 worked:
Jest output

<project>/node_modules/react-spinners/dist/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import BarLoaderComponent from "./
BarLoader";
                                                                                                    ^^^^^^^^^^^^^^^^^^

    SyntaxError: Unexpected identifier

      1 | import React from 'react';
      2 | 
    > 3 | import { GridLoader } from 'react-spinners';
        | ^
      4 | 
      5 | import styles from './GridSpinner.module.scss';

@davidhu2000
Copy link
Owner

this is so weird. these are the only changes for 0.5.13

53e943b

a51b972

93ca51c

No idea how these commits are causing this issue.

Can you try to import like this and see if the issue persists?

import GridLoader from 'react-spinners/GridLoader';

@814k31
Copy link

814k31 commented Jul 26, 2019

@davidhu2000 importing like import GridLoader from 'react-spinners/GridLoader'; seems to work for me

@davidhu2000
Copy link
Owner

sounds like there is an issue with the index.ts exports. I will look into that

@amsully
Copy link
Author

amsully commented Jul 26, 2019

Within a minimal replication repo, the change of import style resolved the problem, but our larger project has react-spinners throw an error when importing react.

minimal replication repo/instructions: https://github.com/amsully/react_spinner_issue

Error thrown when changing all imports in larger project:
Screen Shot 2019-07-26 at 9 57 29 AM

@davidhu2000
Copy link
Owner

davidhu2000 commented Jul 28, 2019

I figured it out. Looks like the dist folder accidentally got pushed up as part of this patch. That folder includes all the typescript stuff, and there look to be some bugs compiling the code. That explains why v0.6.0-alpha.1 has similar problems.

This is what caught my eye finally.

[...]/ensemble/node_modules/react-spinners/dist/index.js:1

All the desired files should be in the root folder, to allow for individual imports. I cloned ensemble, installed v0.5.13. Saw jest tests failed. Went into node modules and deleted /dist folder, then was able to run the tests successfully.

I was in the middle of converting the project to typescript and must have switched between branches to push up this patch. Version 0.5.12 does not have this issue.

This will be something I fix in v0.6.x. But for now, I don't think there are any actions to take.

This seems to only affect meteor for some reason. I was able to minify without issues using react.

@davidhu2000
Copy link
Owner

looks like the problem is in tsconfig:

"module": "esnext",

need to be changed to

"module": "commonjs",

@davidhu2000
Copy link
Owner

Going to keep this open until we can confirm this is resolved

@davidhu2000 davidhu2000 reopened this Jul 28, 2019
openjck added a commit to mozilla/ensemble that referenced this issue Jul 31, 2019
react-spinners is not updated due to the following bug:
davidhu2000/react-spinners#74
@shaibam
Copy link

shaibam commented Aug 8, 2019

Will updating to the newest version fix it? or do I have to manually change the tsconfig?

@davidhu2000
Copy link
Owner

seems like there are a lot of users with this problem. so I'm going to deprecate this version. Hoping to release 0.6.0 soon, just promoted it to beta. 😄

@davidhu2000
Copy link
Owner

ok, 0.5.13 is deprecated. tagged 0.5.12 as latest.

@davidhu2000
Copy link
Owner

This has been fixed in v0.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants