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: remove duplicate prettier configs #125

Closed

Conversation

DanielSchaffer
Copy link
Contributor

@DanielSchaffer DanielSchaffer commented Jan 27, 2020

Updates linting configurations, dependencies, and runtime versions to allow using latest eslint-plugin-prettier

  • upgraded babel-eslint, eslint, prettier, pretty-quick to match versions used by eslint-plugin-prettier
  • added @babel/core, required by eslint
  • set NodeJS runtime to 10.18.1: @babel/core does not support the existing build version (8.7), and the 8.x line is EOL as of 12/31/2019. 10.18.1 is the most up to date from 10.x, which is still in extended maintenance
  • temporarily disabled eslint rules triggered by existing code to avoid touching source files in this PR - to be fixed in Fix errors from temporarily disabled eslint rules #126

@DanielSchaffer
Copy link
Contributor Author

DanielSchaffer commented Jan 27, 2020

Looks like the Netlify build is failing due to it attempting to install fsevents, which is not compatible with Linux - this should be marked as an optional dependency, and I'm wondering if updating the version of Yarn used (appears to be 1.3, current is 1.21`) would allow it to behave correctly.

@DanielSchaffer DanielSchaffer force-pushed the daniel/remove-duplicate-prettier-configs branch from 9af118d to 651b00b Compare January 27, 2020 20:06
…work

- initially disable newly breaking rules
- use NodeJS 10.18.1 (latest 10.x - LTS/12.x has issues with the fsevents dependency)
- add nvmrc with matching version to circleci build image

closes #37
"eslint-config-prettier": "3.3.0",
"eslint-plugin-instawork": "0.0.2",
"eslint-plugin-prettier": "3.0.0",
"eslint": "5.16.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any breaking changes in this eslint upgrade that may require changes in the custom rules in eslint-plugin-instawork?

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 that I'm aware of - 5.16.0 is the same version used by eslint-plugin-instawork

.eslintrc Outdated Show resolved Hide resolved
"react/destructuring-assignment": "off",
"react/jsx-sort-props": "off",
"react/no-access-state-in-setstate": "off",
"sort-keys": "off"
Copy link
Contributor

Choose a reason for hiding this comment

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

We follow a convention of sorting keys alphabetically. This seems to bypass that if I'm not mistaken?

Copy link
Contributor Author

@DanielSchaffer DanielSchaffer Jan 28, 2020

Choose a reason for hiding this comment

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

Correct, though it is intended to be temporary. Upgrading to eslint-plugin-instawork@0.4.0 resulted in 200+ new linting errors. I decided to leave fixing the errors to different PR - see #126 so this one could be kept as small as possible.

"no-restricted-globals": "off",
"prefer-destructuring": "off",
"react/destructuring-assignment": "off",
"react/jsx-sort-props": "off",
Copy link
Contributor

Choose a reason for hiding this comment

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

We follow a convention of sorting props alphabetically. This seems to bypass that if I'm not mistaken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, though it is intended to be temporary. Upgrading to eslint-plugin-instawork@0.4.0 resulted in 200+ new linting errors. I decided to leave fixing the errors to different PR - see #126 so this one could be kept as small as possible.

Comment on lines +43 to +51
"import/no-cycle": "off",
"lines-between-class-members": "off",
"no-else-return": "off",
"no-restricted-globals": "off",
"prefer-destructuring": "off",
"react/destructuring-assignment": "off",
"react/jsx-sort-props": "off",
"react/no-access-state-in-setstate": "off",
"sort-keys": "off"
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 some of them should be added to eslint-plugin-instawork instead of hyperview repo individually. Could you also consult @flochtililoch for these changes? Some of them were intentionally added in eslint-plugin-instawork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my other comments - these are intended to be temporary.

@DanielSchaffer DanielSchaffer force-pushed the daniel/remove-duplicate-prettier-configs branch from 5345ad0 to a404e4d Compare January 28, 2020 16:14
@flochtililoch
Copy link
Collaborator

Thanks for your first contribution to Hyperview repo, @DanielSchaffer!
As this update requires some code changes that are spread out across several PRs, it would be good to have it opened against an integration branch, so that we can continue iterating on master branch in parallel. The final integration in master can happen once the refactor is complete.
Note: each intermediate PR in your integration branch don't necessarily need to build, as long as the final PR to master does - so you won't need to temporarily disable any linting rules just to satisfy CI.

@flochtililoch flochtililoch removed their request for review April 13, 2020 16:15
@flochtililoch
Copy link
Collaborator

Some of the changes here overlap with #215, closing this for 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