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

fix devBuild #877

Merged
merged 7 commits into from
Jul 1, 2017
Merged

fix devBuild #877

merged 7 commits into from
Jul 1, 2017

Conversation

chenqingspring
Copy link
Contributor

@chenqingspring chenqingspring commented Jun 22, 2017

This change is Reviewable

@justin808
Copy link
Member

@mjhenkes This looks great. I'll examine this tomorrow. What are the reproduction steps to see the issue?

@Conturbo can you try this out?

@mjhenkes
Copy link

mjhenkes commented Jun 30, 2017

@justin808 Assuming a webpack config that looks something like this.

Then build assets for production (NODE_ENV=production webpack --config webpack.config.js) and the console will log "Webpack dev build for Rails".
devBuild is always set to true because env (the list of environment variables) will never equal 'production'

@chenqingspring I'd recommend checking const devBuild = configEnv !== 'production'; to stay inline with the the fallbacks specified in the code.

@chenqingspring
Copy link
Contributor Author

@mjhenkes It makes more sense :)

@justin808
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@justin808
Copy link
Member

@chenqingspring Please add a changelog.md entry and I'll merge this.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@justin808
Copy link
Member

@chenqingspring

  1. CI failed linting
  2. Please format the message for 8.0.4 as the release number EXACTLY like the other entries. And be sure to update both the top and the bottom of the file.
Finished in 1.38 seconds (files took 2.25 seconds to load)
1 example, 0 failures
Completed all RSpec tests
cd /home/travis/build/shakacode/react_on_rails && yarn run eslint
yarn run v0.24.6
$ eslint . 
/home/travis/build/shakacode/react_on_rails/webpackConfigLoader.js
  58:1  error  Trailing spaces not allowed  no-trailing-spaces
✖ 1 problem (1 error, 0 warnings)

@justin808
Copy link
Member

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


CHANGELOG.md, line 12 at r3 (raw file):
Like this format, and be sure to see how the version number corresponds to the bottom of the this file.

[8.0.2]

Fixed


Comments from Reviewable

@@ -8,6 +8,10 @@ Changes since last non-beta release.

*Please add entries here for your pull requests.*

## [8.0.4]
Copy link
Member

@justin808 justin808 Jul 1, 2017

Choose a reason for hiding this comment

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

see the bottom of the file for how you do the diff of 8.04 and 8.03, @chenqingspring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I didn't notice the references :(

but the build failed again..

@justin808
Copy link
Member

Thanks @chenqingspring!

@justin808 justin808 merged commit 9d8a987 into shakacode:master Jul 1, 2017
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.

3 participants