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

move babel requirements into dependencies #71

Closed
wants to merge 1 commit into from
Closed

move babel requirements into dependencies #71

wants to merge 1 commit into from

Conversation

modosc
Copy link
Contributor

@modosc modosc commented Nov 23, 2016

we use webpack and babel-loader and explicitly exclude /node_modules/ from babel-loader:

    test: /\.js$/,
    exclude: /node_modules/,
    loader: 'babel-loader',

we now get this error:

10:54:49 client.1 | ERROR in ./~/react-responsive/src/mediaQuery.js
10:54:49 client.1 | Module parse failed: /Users/jon/git/instaflux/node_modules/react-responsive/src/mediaQuery.js Unexpected token (66:2)
10:54:49 client.1 | You may need an appropriate loader to handle this file type.
10:54:49 client.1 | SyntaxError: Unexpected token (66:2)
10:54:49 client.1 |     at Parser.pp$4.raise (/Users/jon/git/instaflux/node_modules/webpack/node_modules/acorn/dist/acorn.js:2221:15)

that line is using the es6 spread operator which requires specific babel plugins / configuration.

we do use a babel config that supports this so i change my babel-loader to run for this module only (per this)

    test: /\.js$/,
    exclude: /node_modules(?!\/react-responsive)/,
    loader: 'babel-loader',

then we got this error:

11:00:50 server.1 | Module build failed: Error: Couldn't find preset "stage-0" relative to directory "/Users/jon/git/instaflux/node_modules/react-responsive"
11:00:50 server.1 |     at /Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/file/options/option-manager.js:299:19
11:00:50 server.1 |     at Array.map (native)
11:00:50 server.1 |     at OptionManager.resolvePresets (/Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/file/options/option-manager.js:270:20)
11:00:50 server.1 |     at OptionManager.mergePresets (/Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/file/options/option-manager.js:259:10)
11:00:50 server.1 |     at OptionManager.mergeOptions (/Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/file/options/option-manager.js:244:14)
11:00:50 server.1 |     at OptionManager.init (/Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/file/options/option-manager.js:374:12)
11:00:50 server.1 |     at File.initOptions (/Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/file/index.js:216:65)
11:00:50 server.1 |     at new File (/Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/file/index.js:139:24)
11:00:50 server.1 |     at Pipeline.transform (/Users/jon/git/instaflux/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
11:00:50 server.1 |     at transpile (/Users/jon/git/instaflux/node_modules/babel-loader/lib/index.js:41:20)
11:00:50 server.1 |  @ ./components/Responsive.js 1:528-570

because those requirements are listed in devDependencies they don't get installed when npm does its build.

this pr moves those into proper deps so that they work.

beyond that the es6 build probably deserves at least a minor version bump (if not major) - going from 1.2.1 -> 1.2.3 broke our app and required digging to solve. react-helmet recently went through something similar..

@modosc
Copy link
Contributor Author

modosc commented Nov 23, 2016

actually there's also this conversation which implies that the js:next build should not just be source code but actually be a fully transpiled / bundled version of the code with import / export statements intact, see the related pr here

@yocontra
Copy link
Owner

Yeah, we shouldn't be pointing at es6 code in the package. main should point at the dist file.

Want to send a PR to fix that?

@modosc
Copy link
Contributor Author

modosc commented Nov 23, 2016

sure, working on it now. this pr isn't working in our app when we do a production build with webpack so it should probably be closed out.

@yocontra yocontra closed this Nov 23, 2016
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

Successfully merging this pull request may close these issues.

None yet

2 participants