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

chore: eslint-plugin migration #1505

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

kresimir-coko
Copy link
Member

Part of liferay/liferay-frontend-projects#677.

I've disabled rules that were spitting errors so that yarn lint returns the same warnings as it does on master.

This is my first time contributing here, so if I overlooked something, let me know

@diegonvs
Copy link
Contributor

When enabling:

✖ 877 problems (231 errors, 646 warnings)
  20 errors and 0 warnings potentially fixable with the `--fix` option.

😢

Also, It emits some warnings like:

Screen Shot 2021-10-15 at 15 13 37

for sorting keys but it's just a warning

ohh, but I saw this commit here and since this project is under maintenance mode I think it's okay to not working in fixing those warns.

So, I think this PR is okay to be merged since it disables the failing rules.

Hey @markocikos, @carloslancha can I hit merge?

Copy link
Contributor

@diegonvs diegonvs left a comment

Choose a reason for hiding this comment

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

Hey @kresimir-coko, could you run npx yarn-deduplicate and squash with the changes in the lockfile? When running in my machine it detected some possible optimizations :)

@kresimir-coko kresimir-coko force-pushed the eslint-plugin-migration branch from a37d08f to c5dda68 Compare October 18, 2021 07:15
@kresimir-coko
Copy link
Member Author

Hey @diegonvs I've ran the command and squashed all commits before force-pushing. So give it another try and lemme know if I need to change anything else

Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

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

@kresimir-coko I tried it out, looks good! yarn lint:

screen

But, I needed to install these packages, that ended up in root package.json. If this the result of something missing on my side, or do we need to update these dependencies?

"devDependencies": {
	...
	"eslint-plugin-notice": "^0.9.10",
	"eslint-plugin-react": "^7.26.1",
	"eslint-plugin-react-hooks": "^4.2.0",
	"eslint-plugin-sort-destructure-keys": "^1.3.5",
	...
}

@kresimir-coko
Copy link
Member Author

@markocikos If this the result of something missing on my side, or do we need to update these dependencies?

I'll investigate a bit later and report back with my findings

@kresimir-coko
Copy link
Member Author

@markocikos so you pulled my changes and just ran yarn lint which added those packages to package.json? I can't seem to replicate that.

@markocikos
Copy link
Member

@kresimir-coko It looks like this was fake news, something was messed up with my repo. After cleaning up node_modules with git clean -dfx, I cannot reproduce #1505 (review).

I did need to do yarn install first, because yarn lint reports that eslint: not found otherwise. This updates yarn lock file. Are you doing something different to enable yarn lint?

@kresimir-coko kresimir-coko force-pushed the eslint-plugin-migration branch from c5dda68 to d02db4d Compare October 19, 2021 05:07
@kresimir-coko
Copy link
Member Author

I don't get the issue with eslint missing, according to package.json it should be present. Here's the steps I took:

  1. git clean -dfx
  2. yarn install - creates a yarn.lock
  3. npx yarn-deduplicate
  4. yarn lint - works without issue

Note: I force-pushed once again because either my initial npx yarn-deduplicate didn't do anything (even though it did modify the yarn.lock file, or I missed something.

Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

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

Works flawlessly now! 👍

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.

3 participants