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

Using devtools with react-stylegudist #456 #569

Merged
merged 2 commits into from
Aug 10, 2017

Conversation

AttackTheDarkness
Copy link
Contributor

The devtool setting in your webpack config will now get passed through for non-production environments, e.g., running the hot-reloading server.

It will still get ignored for production builds.

Docs updated, and added a test.

The `devtool` setting in your webpack config will now get passed through for non-production environments, e.g., running the hot-reloading server.

It will still get ignored for production builds.

Docs updated, and added a test.
@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #569 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   96.04%   96.04%   +<.01%     
==========================================
  Files          99       99              
  Lines        1314     1316       +2     
  Branches      271      271              
==========================================
+ Hits         1262     1264       +2     
  Misses         51       51              
  Partials        1        1
Impacted Files Coverage Δ
scripts/utils/mergeWebpackConfig.js 100% <100%> (ø) ⬆️
src/styles/createStyleSheet.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df1a321...572a4d7. Read the comment docs.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! I have a few suggestions ;-)

@@ -38,3 +38,13 @@ it('should ignore certain Webpack plugins', () => {
expect(result.plugins[0].constructor.name).toBe('UglifyJsPlugin');
expect(result.plugins[1].constructor.name).toBe('MyPlugin');
});

[
Copy link
Member

Choose a reason for hiding this comment

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

It would be almost shorter and much more readable if you write two separate tests without the loop ;-)

// For production builds, we'll ignore devtool settings to avoid
// source mapping bloat.
if (env === 'production') {
safeUserConfig = omit(safeUserConfig, ['devtool']);
Copy link
Member

Choose a reason for hiding this comment

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

I’d define a new const for that, like IGNORE_SECTIONS_PROD for consistency. Or even:

const IGNORE_SECTIONS_ENV = {
  development: [],
  production: ['devtool']
}

In this case you could avoid a condition.

Made a new `IGNORE_SECTIONS_ENV` object to hold environment-specific sections to ignore. `production` will ignore `devtool` as per the original checkin. `development` will pass it through.

Unrolled the test loop into 2 separate tests.
@sapegin sapegin merged commit debcc48 into styleguidist:master Aug 10, 2017
@sapegin
Copy link
Member

sapegin commented Aug 10, 2017

Thanks, merged!

@AttackTheDarkness AttackTheDarkness deleted the devtool branch August 14, 2017 18:30
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