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

build: group webpack & ignore dist & support yarn1.x #2546

Closed
wants to merge 7 commits into from
Closed

build: group webpack & ignore dist & support yarn1.x #2546

wants to merge 7 commits into from

Conversation

curly210102
Copy link

📑 Summary

@Yash-Singh1 Ref: #2456

  • Bulid
    • Don't push dist to GitHub
    • Group webpack files

📏 Design Decisions

  1. Group webpack configs to .webpack folder, and share base config with webpack-merge.
  2. Move html files in dist directory to demo directory
  3. improve husky + lint-stage, change yarn dlx to yarn run, as dlx only supported on version 2.x

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted develop branch

.eslintignore Outdated Show resolved Hide resolved
.eslintignore Outdated
.github/**
yarn.lock
demos/**
**/*.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you ignoring json files ?

Suggested change
**/*.json

Copy link
Author

Choose a reason for hiding this comment

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

eslint+lint-staged think package.json is invalid and refuse commit.
Screen Shot 2021-12-10 at 8 07 36 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Then let's just ignore package.json and not all the json files

Copy link
Author

Choose a reason for hiding this comment

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

In fact, this happens on all json files more than just package.json. For example, I changed cypress.json and add it to stage and exec yarn lint-staged
Screen Shot 2021-12-10 at 8 17 14 PM

But it doesn't throw error when just exec yarn lint

Copy link
Contributor

Choose a reason for hiding this comment

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

What is causing this the lint-staged configuration or the eslint configuration?

Copy link
Author

@curly210102 curly210102 Dec 10, 2021

Choose a reason for hiding this comment

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

In my understanding, ESLint is a javascript linter which is used to lint js/ts/jsx. When exec yarn eslint ., will lint all js files in project. But when ESLint is used with lint-staged, lint-staged will provides it with other types of files, the parser '@babel/eslint-parser' can not work with other types(e.g. json).

And I noticed that jsx was enabled in parserOptions, although I didn't find anywhere have used it.

Thus, I recomand set lint-staged configuration as below

{
  "*.{js,jsx}": ["yarn lint:fix", "git add"],
  "!*.{js,jsx}": ["prettier --write --ignore-unknown", "git add"]
}

Copy link
Author

@curly210102 curly210102 Dec 10, 2021

Choose a reason for hiding this comment

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

lint staged files:

  • js and jsx: lint by ESLint
  • json,md,css,scss,html,yml...: format by prettier

@mmorel-35 What do you think?

@knsv
Copy link
Collaborator

knsv commented Dec 11, 2021

is this ready to be merged?
@mmorel-35 Go ahead and merge when you are happy

@mmorel-35 mmorel-35 mentioned this pull request Dec 11, 2021
3 tasks
@mmorel-35
Copy link
Contributor

I have handled everything in #2551 , thank you @curly210102 for your great contribution

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.

4 participants