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.

Sign up for GitHub

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

Jump to bottom

Use Babel in main process with Webpack build #197

Merged
merged 5 commits into from
Apr 15, 2016
Merged

Use Babel in main process with Webpack build #197

merged 5 commits into from
Apr 15, 2016

Conversation

jhen0409
Copy link
Member

@jhen0409 jhen0409 commented Apr 9, 2016

Related to #139, and just use -r babel-core/register in dev mode.

cc @chentsulin @davej

chentsulin
chentsulin reviewed Apr 9, 2016
View reviewed changes
webpack.config.electron.js
})
);
config.externals.push('electron');
config.target = 'node';
Copy link
Member

Choose a reason for hiding this comment

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

should use 'electron' as target

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed :)

@jhen0409 jhen0409 force-pushed the webpack-main branch 2 times, most recently from 96ff89a to 58a66aa Compare April 9, 2016 16:46
jhen0409
jhen0409 reviewed Apr 9, 2016
View reviewed changes
webpack.config.production.js
@@ -15,7 +15,7 @@ config.entry = './app/index';

config.output.publicPath = '../dist/';

config.module.loaders.push({
config.module.loaders = config.module.loaders.concat([{
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks .production.js and .electron.js will have problem of object reference in package.js, so I handle these with array.concat.

@jhen0409 jhen0409 force-pushed the webpack-main branch 6 times, most recently from 463c312 to ae07363 Compare April 10, 2016 17:19
@chentsulin
Copy link
Member

I just wonder whether babel-regiseter is good enough for development..If we use some webpack specified feature in production, we can't directly apply them to development env.

chentsulin
chentsulin reviewed Apr 12, 2016
View reviewed changes
package.json
@@ -10,9 +10,11 @@
"test-e2e": "cross-env NODE_ENV=test mocha --compilers js:babel-core/register --require ./test/setup.js --require co-mocha ./test/e2e.js",
"lint": "eslint app test *.js",
"hot-server": "babel-node server.js",
"build": "cross-env NODE_ENV=production babel-node ./node_modules/.bin/webpack --config webpack.config.production.js --progress --profile --colors",
"build-electron": "cross-env NODE_ENV=production babel-node ./node_modules/.bin/webpack --config webpack.config.electron.js --progress --profile --colors",
Copy link
Member

Choose a reason for hiding this comment

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

build-main and build-renderer are nicer.

@jhen0409
Copy link
Member Author

If we use some webpack specified feature in production, we can't directly apply them to development env.

Yes, I can think of case is webpack loader, but it can be resolved by babal-plugin-webpack-loaders.

In addition, I recommend webpack build of main process is for production. @chentsulin What do you think?

@chentsulin
Copy link
Member

I'm ok for that. I don't know why but there is a regression about #192. It seems window will open before content ready.

@jhen0409
Copy link
Member Author

It seems window will open before content ready.

It's ERR_CONNECTION_REFUSED error? It happens sometimes.

The npm run hot-server used babel-node (#195), so it's time to start may later than main process.

@chentsulin
Copy link
Member

Could we somehow fix ERR_CONNECTION_REFUSED to improve user experience here?

@jhen0409
Copy link
Member Author

Use node -r babel-register will be faster, it just not has some options and require babel-polyfill.

chentsulin
chentsulin reviewed Apr 15, 2016
View reviewed changes
package.json
@@ -8,7 +8,7 @@
"test": "cross-env NODE_ENV=test mocha --compilers js:babel-register --recursive --require ./test/setup.js test/**/*.spec.js",
"test-watch": "npm test -- --watch",
"test-e2e": "cross-env NODE_ENV=test mocha --compilers js:babel-register --require ./test/setup.js --require co-mocha ./test/e2e.js",
"lint": "eslint app test *.js",
"lint": "eslint app test *.js --ignore-pattern main.js",
Copy link
Member

Choose a reason for hiding this comment

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

Why not put this in a .eslintignore?

@chentsulin
Copy link
Member

I am almost ready to merge this PR. @jhen0409 Could you move --ignore-pattern to a .eslintignore? Thanks a lot.

@chentsulin chentsulin merged commit 2152ee3 into electron-react-boilerplate:master Apr 15, 2016
@chentsulin chentsulin mentioned this pull request Apr 15, 2016
Closed
@jhen0409 jhen0409 deleted the webpack-main branch April 15, 2016 11:55
@jhen0409 jhen0409 mentioned this pull request Dec 24, 2016
Closed
@aprilmintacpineda aprilmintacpineda mentioned this pull request Sep 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhen0409 @chentsulin