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

Add watch support for stylesheets in packages #8380

Merged
merged 3 commits into from
Aug 3, 2018

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Aug 2, 2018

Description

closes #8343

Scss files in packages are not rebuilt while running npm run dev. This PR add watch support for scss files in packages.

How has this been tested?

  • Tested editing both scss and js files in the editor package while npm run dev was active and observed that the changes were then visible in the browser after reloading the page.
  • Tested editing non-package files while npm run dev was active and observed no adverse side-effects
  • Tested npm run build and observed no adverse side-effects

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@talldan talldan added the [Type] Build Tooling Issues or PRs related to build tooling label Aug 2, 2018
@talldan talldan self-assigned this Aug 2, 2018
@talldan talldan requested a review from youknowriad August 2, 2018 04:59
@@ -24,7 +24,7 @@ const exists = ( filename ) => {

// Exclude deceitful source-like files, such as editor swap files.
const isSourceFile = ( filename ) => {
return /.\.js$/.test( filename );
return /.\.(js|scss)$/.test( filename );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we'd need some kind of rule excluding the components package scss from being watched, since that will end up being watched by both webpack and this script.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine :) it will generate a new CSS for the package but not use it in the webpack build.

@talldan talldan force-pushed the add/watch-support-for-stylesheets-in-packages branch 2 times, most recently from b25c9b3 to 8780532 Compare August 2, 2018 05:09
@talldan talldan force-pushed the add/watch-support-for-stylesheets-in-packages branch from 8780532 to cd33c9e Compare August 2, 2018 05:11
@talldan talldan changed the title Add/watch support for stylesheets in packages Add watch support for stylesheets in packages Aug 2, 2018
accumulator.jsFiles.add( filePath );
} else if ( isScssFile( filePath ) ) {
const pkgName = getPackageName( filePath );
const pkgPath = path.resolve( PACKAGES_DIR, pkgName );
Copy link
Contributor

Choose a reason for hiding this comment

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

In another PR #8287 I have the use-case for multiple SASS entry points (multiple CSS outputs) per package. Do you have some ideas on how this would play together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The simplest way I can think of is to add some kind of config, potentially as a custom property to the package.json that could be picked up by the build scripts:

{
  scssEntryPoints: [
    'style.scss',
    'some/other/style.scss'
  ],
}

Either that or some kind of naming convention e.g. styles.main.scss.

I think the config idea is safer.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@talldan talldan merged commit 1de0887 into master Aug 3, 2018
@talldan talldan added this to the 3.5 milestone Aug 3, 2018
@talldan talldan deleted the add/watch-support-for-stylesheets-in-packages branch August 3, 2018 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In the editor package, scss files are not rebuilt while running npm run dev
2 participants