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

Packages: Mark build-styles as side-effectful #19535

Merged
merged 3 commits into from
Jan 13, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jan 9, 2020

Description

In Automattic/wp-calypso#38554 we observed some styling regressions when importing component styles from @wordpress/components/build-styles. This appears to be a regression introduced in #18911. Styles are side-effectful, and should be declared as such. Packages that include build-styles intended for use from the published package must be marked as such. The webpack docs on tree shaking mention this:

Note that any imported file is subject to tree shaking. This means if you use something like css-loader in your project and import a CSS file, it needs to be added to the side effect list so it will not be unintentionally dropped in production mode:

source

This PR marks build-style/** as having side-effects for packages that were observed to produce a build-style directory and were marked sideEffects: false. An alternative would be to add *.css and *.scss to the list, however.

How has this been tested?

When manually applying the change in this PR to the installed package where the regression was observed, the issue is fixed.

Types of changes

Bug fix (non-breaking change which fixes an issue)

  • Fixes a regression published in @wordpress/[email protected] that would prevent some build tools from including styles provided in the packages build-styles directory.

  • Fixes a regression published in @wordpress/[email protected] that would prevent some build tools from including styles provided in the packages build-styles directory.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@sirreal sirreal force-pushed the fix/build-style-side-effects branch from ab79a68 to d5c5f89 Compare January 9, 2020 17:22
@sirreal sirreal self-assigned this Jan 9, 2020
@sirreal sirreal added [Package] Block library /packages/block-library [Package] Components /packages/components [Type] Bug An existing feature does not function as intended npm Packages Related to npm packages Needs Technical Feedback Needs testing from a developer perspective. labels Jan 9, 2020
@sirreal sirreal marked this pull request as ready for review January 9, 2020 17:29
@sirreal sirreal requested review from ockham and jsnajdr January 9, 2020 17:30
@sirreal sirreal requested review from senadir and aduth January 9, 2020 17:34
@sirreal sirreal changed the title Packages: Build-styles are side-effectful Packages: Mark build-styles as side-effectful Jan 9, 2020
@ockham
Copy link
Contributor

ockham commented Jan 10, 2020

Here's some concrete testing steps:

diff --git a/webpack.config.js b/webpack.config.js
index e4e2ba6505..144b093349 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -150,7 +150,7 @@ const webpackConfig = {
                'entry-login': [ path.join( __dirname, 'client', 'landing', 'login' ) ],
                'entry-gutenboarding': [ path.join( __dirname, 'client', 'landing', 'gutenboarding' ) ],
        },
-       mode: isDevelopment ? 'development' : 'production',
+       mode: 'production',
        devtool: process.env.SOURCEMAP || ( isDevelopment ? '#eval' : false ),
        output: {
                path: path.join( __dirname, 'public', extraPath ),
  • npm ci && npm start
  • Access calypso.localhost:3000/gutenboarding
  • Fill in the two questions ("My site is about", "It's called")
  • Look at the domain picker in the header. Its styling is broken (see screenshots below):

Screen Shot 2020-01-09 at 16 44 50

Screen Shot 2020-01-09 at 16 44 38

  • Stop Calypso (Ctr+C in the console where it's running), and open its node_modules/ directory. Modify the package.json files in both @wordpress/block-library and @wordpress/components according to this PR. Restart Calypso (npm start).
  • Access calypso.localhost:3000/gutenboarding again, and verify that the header's and domain selector's styling is fixed 🎉

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Confirming that this fixes the issue we saw by following these instructions: #19535 (comment)

@gziolo
Copy link
Member

gziolo commented Jan 13, 2020

Only those 2 packages are affected?

@gziolo gziolo merged commit 701c277 into master Jan 13, 2020
@gziolo gziolo deleted the fix/build-style-side-effects branch January 13, 2020 08:29
@sirreal
Copy link
Member Author

sirreal commented Jan 13, 2020

Only those 2 packages are affected?

I compiled the packages (npm run build:pacakges I believe), then I looked for pacakges that included a build-style directory. Then I checked whether these packages had sideEffects: false and arrived at these two packages. Based on this I believe these are the affected packages.

sirreal added a commit to Automattic/wp-calypso that referenced this pull request Jan 13, 2020
Workaround for regression in package.
See:
WordPress/gutenberg#19535
gziolo pushed a commit that referenced this pull request Jan 13, 2020
* Declare sideEffects for build-style

* fixup! Declare sideEffects for build-style

* Add CHANGELOG entries
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Technical Feedback Needs testing from a developer perspective. npm Packages Related to npm packages [Package] Block library /packages/block-library [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants