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

WP-Scripts Ignores Root Stylelint Configuration on Build & Start #15056

Closed
isvictorious opened this issue Apr 18, 2019 · 9 comments
Closed

WP-Scripts Ignores Root Stylelint Configuration on Build & Start #15056

isvictorious opened this issue Apr 18, 2019 · 9 comments
Labels
[Tool] WP Scripts /packages/scripts

Comments

@isvictorious
Copy link

Describe the bug
I am attempting an advanced configuration of @wordpress/scripts

Override instructions for stylelint and eslint in @wordpress/scripts are to follow stylelint & eslint handbooks. Both state that root configurations should be used.

When running wp-scripts lint-style or wp-scripts lint-js it works as expected using root configuration.

When running wp-scripts build it is referencing default configurations

To reproduce
Steps to reproduce the behavior:

  1. Create a new project and install @wordpress/scripts
  2. Add a custom stylelint configuration to your root directory
  3. Create rules that violate WP core CSS standards but not custom stylelint
  4. You will get core CSS errors

Expected behavior
Expecting root linting configurations to take precedent on build and start

@swissspidy swissspidy added the [Tool] WP Scripts /packages/scripts label Apr 21, 2019
@gziolo gziolo added the [Status] Needs More Info Follow-up required in order to be actionable. label Apr 23, 2019
@gziolo
Copy link
Member

gziolo commented Apr 23, 2019

Expecting root linting configurations to take precedent on build and start

Can you explain what do you exactly expect to happen when you run build or start? Why there should be any relationship between those 2 commands and wp-scripts lint-style or wp-scripts lint-js?

@isvictorious
Copy link
Author

Expecting root linting configurations to take precedent on build and start

Can you explain what do you exactly expect to happen when you run build or start? Why there should be any relationship between those 2 commands and wp-scripts lint-style or wp-scripts lint-js?

Expecting wp-scripts build, wp-scripts start, wp-scripts lint-style, & wp-scripts lint-js to all use same configurations for ESlint & Stylelint.

When running build or start I get CSS errors of unrecognized word for my SCSS. That is because the WP-Scripts configuration doesn't recognize my root stylelint or ESlint configuration.

One could get zero errors on linting commands but errors on start and build. Or the other way around. This is confusing.

@gziolo
Copy link
Member

gziolo commented Apr 23, 2019

When running build or start I get CSS errors of unrecognized word for my SCSS. That is because the WP-Scripts configuration doesn't recognize my root stylelint or ESlint configuration.

Do you import scss files in your JavaScript code? @fabiankaegy is working on adding support for it in #14847. See the parent issue as well: #14801.

@isvictorious
Copy link
Author

I see this separate from #14847 or #14801. WP-Scripts should follow standard of using ESlint or Stylelint in root folder regardless of SCSS or CSS. One may want to add additional rules or ignore rules in their wp-scripts config.

But for SCSS I'm importing scss by extending the WP-Scripts Webpack following Fabian's example on #14590. This example.

I do not see a way to modify the Webpack to use my Stylelint or ESlint configuration on build or start.

@gziolo
Copy link
Member

gziolo commented Apr 24, 2019

@isvictorious - can you share your code. I admit that I don't understand what do you expect from the build script. At the moment it's role is limited to building the production version of JavaScript code from the provided source file. I don't quite understand what role ESLint of Stylelint would play here? Can't you pipe those commands instead: wp-scripts lint-js && wp-scripts build?

@isvictorious
Copy link
Author

isvictorious commented Apr 24, 2019

Code is here: https://github.com/isvictorious/demo-video-block

Clarifying: When you run npm start in most tools, it is watching your code for errors. That is how it works for Create-React-App and various other tools. However, the WP-Scripts npm start is reading the wrong lint configs.

This means npm start is not in line with npm run lint:style & JS lint configs. You are then checking your code against 2 style guides.

Screenshot: Result when I run npm run lint:style. 100% error free because it is using my root lint settings for Stylelint and ESlint
https://cl.ly/4f4acaec2ddd

Screenshot: Result when I run npm run build. Errors because it is using WordPress CSS lint standards from WP-Scripts Webpack configuration.
https://cl.ly/2a31656d841f

I'm expecting that the entire configuration regardless of commands should respect the root stylelint or ESlint configuration. This is a standard according to stylelint / ESlint docs.

@gziolo
Copy link
Member

gziolo commented Apr 25, 2019

Screenshot: Result when I run npm run build. Errors because it is using WordPress CSS lint standards from WP-Scripts Webpack configuration.
https://cl.ly/2a31656d841f

I don't think the issue comes from the linter. I don't see it integrated in the webpack config shared:
https://github.com/isvictorious/demo-video-block/blob/feature/wp-scripts/webpack.config.js#L17-L46

@isvictorious
Copy link
Author

isvictorious commented Apr 25, 2019

The issue is with WP-Scripts configuration in the scripts folder.

The lint-style.js file checks for an existing stylelint configuration https://github.com/WordPress/gutenberg/blob/master/packages/scripts/scripts/lint-style.js starting at line 20

start and build do not. They do use utils which checks for existing babel configurations. They should also check for an existing stylelint configuration & ESlint configuration at root.

It is incorrect for start build lint-style & other linters to reference differing configurations.

@gziolo
Copy link
Member

gziolo commented Apr 26, 2019

It is incorrect for start build lint-style & other linters to reference differing configurations.

To make it clear. build and start commands don't provide integration with linters as of today. I see that create-react-app has ESLint verification included as a step before Babel processing:

https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/config/webpack.config.js#L312-L334

If you feel like this should be included in wp-scripts as well, feel free to open a sperate issue. For stylelint it's probably the best to comment on #14801 to have it considered as well.

I'm closing this issue as there is nothing to fix on Gutenberg's side.

@gziolo gziolo closed this as completed Apr 26, 2019
@gziolo gziolo removed the [Status] Needs More Info Follow-up required in order to be actionable. label Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts
Projects
None yet
Development

No branches or pull requests

3 participants