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 Stylelint checks and checks output consistency #377

Merged
merged 100 commits into from
Apr 21, 2023
Merged

Conversation

Josehower
Copy link
Contributor

@Josehower Josehower commented Apr 11, 2023

Following a similar approach to the ESLint check, this PR adds a check for Stylelint to Preflight. The implemented features are:

  • Check for warnings and errors in css, sass, scss, less, js, tsx and jsx files
  • Check the stylelint.config.js file exist and match the template in upleveled-stylelint-config package
  • UpLeveled Stylelint Config package is updated
  • Checks are only performed when the project is a React project

In addition This PR updates outputs for ESLint check and Prettier check in order to have a single path format for the output

New UpLeveled Prefilght output:

🚀 UpLeveled Preflight v1.23.3
✔ All changes committed to Git                                                                                                                                                                                                                                      
✔ node_modules/ folder ignored in Git                                                                                                                     
✔ No extraneous files committed to Git                                                                                                                    
✔ No secrets committed to Git                                                                                                                             
✔ Use single package manager                                                                                                                              
✔ Project folder name matches correct format                                                                                                              
✔  No dependency problems                                                                                                                                                                                                                                                                                                                                                
✔ GitHub repo has deployed project link under About                                                                                                       
✖ ESLint
  › ESLint problems found in the following files:
    app/api/hello/route.js

    Open these files in your editor - there should be problems to fix

✖ Stylelint
  › Stylelint problems found in the following files:
    app/globals.css
    app/page.module.css

    Open these files in your editor - there should be problems to fix

✖ Prettier
  › Prettier has not been run in the following files:
    app/api/hello/route.js

    For each of the files above, open the file in your editor and save the file. This will format the file with Prettier, which will cause changes to     
    appear in Git.

    In some very uncommon cases (this probably doesn't apply to you), the mismatch may come from inconsistent end of line characters. Read more here:     
    https://github.com/upleveled/answers/issues/31

✔ ESLint config is latest version
✖ Stylelint config is latest version
  › Your stylelint.config.cjs file does not match the configuration file template - please delete the file and reinstall the config using the instructions on https://www.npmjs.com/package/eslint-config-upleveled
                                                                                                                   
✔ Preflight is latest version                                                    

@github-actions
Copy link

github-actions bot commented Apr 11, 2023

size-limit report 📦

Path Size
dist/preflight.esm.js 7.59 KB (+4.58% 🔺)

src/checks/stylelint.ts Outdated Show resolved Hide resolved
src/checks/stylelint.ts Outdated Show resolved Hide resolved
@Josehower Josehower changed the title Add StyleLint check Add Stylelint check Apr 19, 2023
module.exports = config;`;
} catch (err) {
throw new Error(
`Error reading your stylelint.config.cjs file. Please reinstall the config using the instructions on https://www.npmjs.com/package/eslint-config-upleveled
Copy link
Member

@karlhorky karlhorky Apr 19, 2023

Choose a reason for hiding this comment

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

  1. I guess this error should mention that the config file should be deleted if it exists
  2. Probably we want to mention this for the same error in the ESLint config check
  3. the period can be changed to this pattern: d242231

Copy link
Contributor Author

Choose a reason for hiding this comment

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


if (!stylelintConfigMatches) {
throw new Error(
`Your Stylelint config file stylelint.config.cjs does not match the configuration file template. Please reinstall the config using the instructions on https://www.npmjs.com/package/eslint-config-upleveled
Copy link
Member

@karlhorky karlhorky Apr 19, 2023

Choose a reason for hiding this comment

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

  1. I guess this error should mention that the config file should be deleted if it exists
  2. Probably we want to mention this for the same error in the ESLint config check
  3. the period can be changed to this pattern: d242231

Copy link
Contributor Author

Choose a reason for hiding this comment

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

})) {
const fileContents = await fs.readFile(path, 'utf-8');
if (
/stylelint-disable|stylelint [a-z0-9@/-]+: (0|off)/.test(fileContents)
Copy link
Member

Choose a reason for hiding this comment

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

is the second part of this pattern a valid way to ignore code in Stylelint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/index.ts Outdated
@@ -87,6 +91,6 @@ await new Listr(listrTasks, {
removeEmptyLines: false,
formatOutput: 'wrap',
},
fallbackRenderer: 'verbose',
Copy link
Member

Choose a reason for hiding this comment

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

this is probably a merge mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fallbackRenderer: 'verbose',

@karlhorky
Copy link
Member

new check output:

the output in the description needs to be updated with the Preflight output including Stylelint config check too

This reverts commit 4ad180a.
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved

// If no ESLint problems detected, throw the error
if (!/^\d+ problems?$/.test(lines[lines.length - 2]!)) {
if (!new RegExp(`"${errorKey}":`).test(stdout)) {
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong, it won't fail on warnings

Copy link
Member

Choose a reason for hiding this comment

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

clarification, it looks like errorCount may always exist, but it's relying on an implementation detail to assume that it will always be there when there are problems

Copy link
Member

Choose a reason for hiding this comment

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

instead of checking for two dynamic keys, look at your data when you have no errors, maybe you can see a better way of checking it

src/checks/eslint.ts Outdated Show resolved Hide resolved
src/checks/stylelint.ts Outdated Show resolved Hide resolved
Josehower

This comment was marked as duplicate.

@@ -20,6 +22,8 @@ import {
projectPackageJson,
} from './util/packageJson.js';

const projectDependencies = projectPackageJson.dependencies || {};
Copy link
Member

@karlhorky karlhorky Apr 21, 2023

Choose a reason for hiding this comment

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

I actually like this pattern better than what we have, let's copy it over in these 2 places here too:

Copy link
Member

Choose a reason for hiding this comment

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

done in 560b36a

${(JSON.parse(stdout) as ESLint.LintResult[])
.filter(
(eslintResult) =>
!(eslintResult[errorKey] === 0 && eslintResult[warningKey] === 0),
Copy link
Member

Choose a reason for hiding this comment

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

how would this be the case? what is a a LintResult which has both errorCount and warningCount both of 0? does this happen in reality?

if it does, then this probably needs a comment describing when this is the case

@karlhorky karlhorky merged commit 675c0b4 into main Apr 21, 2023
@karlhorky karlhorky deleted the add-stylelint-check branch April 21, 2023 17:14
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.

None yet

2 participants