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

Webpack style export #613

Merged
merged 5 commits into from Jun 14, 2019
Merged

Webpack style export #613

merged 5 commits into from Jun 14, 2019

Conversation

@kevinkace
Copy link
Contributor

@kevinkace kevinkace commented Jun 10, 2019

Adds option to disable styleExports in Webpack bundle.

Description

Adds a check before adding var style = ... to the Webpack bundle.

Motivation and Context

This value is not always needed, and removing it can reduce the bundle size.

How Has This Been Tested?

Added a test. Checked the output. Defaults to the current behavior.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@codecov
Copy link

@codecov codecov bot commented Jun 10, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #613      +/-   ##
========================================
+ Coverage      99%    99%   +<.01%     
========================================
  Files          48     48              
  Lines        1207   1209       +2     
  Branches      184    185       +1     
========================================
+ Hits         1195   1197       +2     
  Misses         12     12
Impacted Files Coverage Δ
packages/webpack/loader.js 92.59% <100%> (+0.59%) ⬆️

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 1790301...e71f4e3. Read the comment docs.

@tivac
Copy link
Owner

@tivac tivac commented Jun 10, 2019

Idea is good, I've got some more specific feedback that I'll add when I'm not on my phone.

@@ -50,7 +50,9 @@ module.exports = async function(source) {
out.push(`export var ${ident} = ${JSON.stringify(exported[ident])};`);
});

out.push(`export var styles = ${JSON.stringify(result.details.result.css)};`);
if(options.styleExport !== false) {
Copy link
Owner

@tivac tivac Jun 10, 2019

Choose a reason for hiding this comment

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

Can this be added to options as a default with a value of true, so this check could then be if(!options.styleExport)?

Copy link
Contributor Author

@kevinkace kevinkace Jun 10, 2019

Choose a reason for hiding this comment

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

I though adding styleExport : true to processor.js would set the default, but not seeing the expected result.

Copy link
Owner

@tivac tivac Jun 10, 2019

Choose a reason for hiding this comment

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

styleExport isn't something the processor knows anything about, that's a bundler-specific thing that should probably go into plugin.js.

Something like this, probably:

 var options = Object.assign(
    Object.create(null),
    { styleExport : true },
    args
);

(or broken out into const defaults = { ... } or something & then mixed into the options)

Copy link
Contributor Author

@kevinkace kevinkace Jun 10, 2019

Choose a reason for hiding this comment

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

Aha, ok. Thanks.

Copy link
Contributor Author

@kevinkace kevinkace Jun 11, 2019

Choose a reason for hiding this comment

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

Still working on this, maybe.

@TravisBuddy

This comment has been hidden.

@TravisBuddy

This comment has been hidden.

@tivac
Copy link
Owner

@tivac tivac commented Jun 11, 2019

Aww crap @kevinkace I led you astray. For some reason I though the Plugin/Loader shared options but they 100% don't.

@kevinkace
Copy link
Contributor Author

@kevinkace kevinkace commented Jun 11, 2019

Ahh ok thanks! I can keep digging to see where a default can go.

@kevinkace
Copy link
Contributor Author

@kevinkace kevinkace commented Jun 14, 2019

A little ugly, but looks like the recommended way to add defaults with loader-utils.

tivac
tivac approved these changes Jun 14, 2019
@tivac tivac merged commit 8fc3ae1 into tivac:master Jun 14, 2019
8 checks passed
@tivac
Copy link
Owner

@tivac tivac commented Jun 14, 2019

I'll cut a release in a bit here, thanks for sticking with it!

@tivac
Copy link
Owner

@tivac tivac commented Jun 17, 2019

Took me longer than hoped for, but this is now released as @modular-css/webpack@24.1.0. Thanks again for all your hard work @kevinkace!

@kevinkace
Copy link
Contributor Author

@kevinkace kevinkace commented Jun 17, 2019

Updating our builds rn! thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants