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

style(eslint): align root and example with the same configuration #1575

Merged
merged 33 commits into from
Oct 18, 2021

Conversation

lisabaut
Copy link
Contributor

@lisabaut lisabaut commented Oct 13, 2021

Description

Hi there 👋

As mentioned in this PR here https://github.com/react-native-mapbox-gl/maps/pull/1486 I said I would look into the linting issue (running yarn lint from root-level causes error in example folder).

I ended up with a refactoring of the current eslint and tslint set-up which now ensures that one configuration works in all JS and TS files.

This PR does:

  • fix the linting script error mentioned above
  • remove all eslint and tslint packages from example to root
  • align the one eslintrc configuration with the same rules as before for JS from the root folder
  • align the one eslintrc configuration with the same rules as before for TS/X files from example folder
  • change script yarn lint to lint all files in all folders with one configuration
  • lint all files in all folder with the same rules
  • fix all errors
  • fix some warnings

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I updated the documentation yarn generate
  • I mentioned this change in CHANGELOG.md
  • I updated the typings files (index.d.ts)
  • I added/ updated a sample (/example)

Questions:

  • Let me know if you are missing some JS rules from the deleted eslintrc file of the example folder :) I decided to take all JS rules from eslintrc of the root folder
  • I needed to update cocoapods in order to make the iOS simulator run, after that the file project.pbxproj was changed, please see the commit here. I assume, I need to revert this commit?
  • after running yarn generate the file for OfflineManager.md was changed, is this correct?
  • does this repo use yarn or npm ?
  • I assume the script yarn format and yarn lint are doing the same now with this PR and one can be removed?

error: Expected an assignment or function call and instead saw an expression: eslintno-unused-expressions, see https://eslint.org/docs/rules/no-unused-expressions
@ferdicus
Copy link
Member

hey @lisabaut, pretty cool - thanks for taking care of this 👍🏿

Some notes:
When I run this in root, I still get two errors 🤔

  • javascript/utils/animated/AnimatedRouteCoordinatesArray.js
    4:3 error convertDistance not found in '@turf/helpers' import/named
  • example/src/examples/Annotations/CustomCallout.tsx
    3:9 error Feature not found in '@turf/helpers' import/named

Let me know if you are missing some JS rules from the deleted eslintrc file of the example folder :) I decided to take all JS rules from eslintrc of the root folder

Nah, all good 👍🏿 We can extend when we find something missing later on

I needed to update cocoapods in order to make the iOS simulator run, after that the file project.pbxproj was changed, please see the commit here. I assume, I need to revert this commit?

Yes, please revert - that's taken care off by the upgrade to RN 0.66 PR (#1570 )

after running yarn generate the file for OfflineManager.md was changed, is this correct?

That was probably forgotten during another PR - you can also revert that, as it's taken care off by #1577

does this repo use yarn or npm ?

We're trying to be npm/yarn indifferent - however there are some instances, like within package.json where npm is used to reference scripts within scripts. If you find occurrences like that and can change them to run without referencing npm or yarn specifically feel free to change them 👍🏿

I assume the script yarn format and yarn lint are doing the same now with this PR and one can be removed?

Correct, feel free to remove one.

Running yarn lint just from the example folder would be nice - but maybe for a follow up PR 🙇🏿

Again, thank you very much for your contribution 🙇🏿

Cheers 🍻

@lisabaut
Copy link
Contributor Author

Some notes: When I run this in root, I still get two errors 🤔

* `javascript/utils/animated/AnimatedRouteCoordinatesArray.js`
  `4:3  error  convertDistance not found in '@turf/helpers'  import/named`

* `example/src/examples/Annotations/CustomCallout.tsx`
  `3:9   error    Feature not found in '@turf/helpers'      import/named`

That is weird. I am not able to reproduce these errors.
I ran yarn purge in the example folder again and deleted the node modules from the root folder.
Then yarn and yarn lint in the root folder => no errors (but the expected 105 warnings)
Then cd into example folder and yarn and cd .. && yarn lint again => same result

Any idea what I am missing here?
Thanks.

@ferdicus
Copy link
Member

Some notes: When I run this in root, I still get two errors 🤔

* `javascript/utils/animated/AnimatedRouteCoordinatesArray.js`
  `4:3  error  convertDistance not found in '@turf/helpers'  import/named`

* `example/src/examples/Annotations/CustomCallout.tsx`
  `3:9   error    Feature not found in '@turf/helpers'      import/named`

That is weird. I am not able to reproduce these errors. I ran yarn purge in the example folder again and deleted the node modules from the root folder. Then yarn and yarn lint in the root folder => no errors (but the expected 105 warnings) Then cd into example folder and yarn and cd .. && yarn lint again => same result

Any idea what I am missing here? Thanks.

hmm 🤔, weird, yeah I'm doing the same thing....

only additional step I ran was rm -rf node_modules in root as yarn purge only takes care of /example

let's merge this - it's already better than it previously was :)

Maybe, if you find the time: We do not have a linting workflow - maybe you'd like to add one in the future?

Cheers 🍻

@ferdicus ferdicus merged commit 0d7435c into rnmapbox:master Oct 18, 2021
@lio-ioki
Copy link

Maybe, if you find the time: We do not have a linting workflow - maybe you'd like to add one in the future?

Sure, with running the tests as well I suppose, as I could not find a workflow for this script either.

@ferdicus
Copy link
Member

Maybe, if you find the time: We do not have a linting workflow - maybe you'd like to add one in the future?

Sure, with running the tests as well I suppose, as I could not find a workflow for this script either.

That's included in the android workflow, see:
https://github.com/react-native-mapbox-gl/maps/blob/0d7435c76bbc5e9099cc8a3d5380f9f0065fcca6/.github/workflows/android-actions.yml#L17

Not saying those can't be improved though.
Building could be decoupled from testing for example.

Thanks again 🙇🏿

@veb-ioki
Copy link
Contributor

Some notes: When I run this in root, I still get two errors 🤔

* `javascript/utils/animated/AnimatedRouteCoordinatesArray.js`
  `4:3  error  convertDistance not found in '@turf/helpers'  import/named`

* `example/src/examples/Annotations/CustomCallout.tsx`
  `3:9   error    Feature not found in '@turf/helpers'      import/named`

That is weird. I am not able to reproduce these errors. I ran yarn purge in the example folder again and deleted the node modules from the root folder. Then yarn and yarn lint in the root folder => no errors (but the expected 105 warnings) Then cd into example folder and yarn and cd .. && yarn lint again => same result
Any idea what I am missing here? Thanks.

hmm 🤔, weird, yeah I'm doing the same thing....

only additional step I ran was rm -rf node_modules in root as yarn purge only takes care of /example

let's merge this - it's already better than it previously was :)

Maybe, if you find the time: We do not have a linting workflow - maybe you'd like to add one in the future?

Cheers 🍻

Just FYI, now that the PR is merged, I pulled and tested on my machine:
root => yarn purge, yarn, yarn lint => no errors, only loads of warnings
cd example => yarn purge, yarn => cd to root again & yarn lint => no errors, only loads of warnings

@lisabaut lisabaut deleted the fix/lintings branch October 20, 2021 07:50
@lisabaut lisabaut mentioned this pull request Oct 21, 2021
6 tasks
sdacunha pushed a commit to sdacunha/maps that referenced this pull request Oct 25, 2021
…mapbox#1575)

* fix: remove deprecated extend

see https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md#version-800-2021-02-21

* chore: move lint packages to root

* refactor: move prettierrc to root

* refactor: move tsconfig to root

* chore: align typescript version with root

* refactor: align one eslintrc withboth rules

* style: run lint on root with same rules from example

* fix: ts errors in plugn folder withMapbox

* chore: run lint on all levels

* style: run lint in example with same rules

* fix: eslint errors in example/scenes/Home

* fix: missing Proptypes in SetUserLocationMode

* fix: false errors for used-before-defined in tsx

* style: disable eslint in example test e22 file

* fix: disable prop-type validation for TSX

* fix: missing proptype for AnnotationContent

* fix: eslintno-unused-expressions

error: Expected an assignment or function call and instead saw an expression: eslintno-unused-expressions, see https://eslint.org/docs/rules/no-unused-expressions

* fix: missing proptypes for ShowPointAnnotation

* fix: TS no-var-requires error for JSON require

* refactor: remove unused eslint disable

* refactor: remove unused eslint-disable

* refactor: remove unused eslint-disable

* refactor: remove unused eslint-disable

* fix: re-add eslint-disable - needed for CI ?

* fix: remove unused import fropm test file

* fix: remove unused import in test file

* fix: incorrect propytes for items in Home.js

- super quick fix :(

* refactor: run yarn generate

* chore: project.pbxproj after cocoapods update ?

* build: update Changelog

* Revert "chore: project.pbxproj after cocoapods update ?"

This reverts commit 68cc15e.

* Revert "refactor: run yarn generate"

This reverts commit a257ddb.

* refactor: remove script `format`
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.

5 participants