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

⬆️ Upgrade ESLint TS parser and plugin #31490

Closed
wants to merge 1 commit into from

Conversation

wcandillon
Copy link
Contributor

I had an issue with the old version of the parser and upgrading it via yarn resolve solved it.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 7, 2021
@pull-bot
Copy link

pull-bot commented May 7, 2021

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against 34f3117

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 1e39d8b

@analysis-bot
Copy link

analysis-bot commented May 7, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,986,662 -227,663
android hermes armeabi-v7a 8,493,909 -219,163
android hermes x86 9,438,943 -226,171
android hermes x86_64 9,380,711 -252,200
android jsc arm64-v8a 10,605,143 -253,340
android jsc armeabi-v7a 10,105,420 +356,904
android jsc x86 10,624,150 -280,524
android jsc x86_64 11,207,825 -306,181

Base commit: d540f88

Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This was required for us as well, adding this (which is the same as these version bumps, essentially) fixed it for us. Would love to see this merged and released

Added to package.json:

  "resolutions": {
    "@typescript-eslint/eslint-plugin": "^4.26.1",
    "@typescript-eslint/parser": "^4.26.1"
  },

With apologies for the direct ping but @PeteTheHeat I see you triggered the facebook github bot for a merge most recently, and this seems like an easy win? Not sure who else to contact to push buttons in these parts :-). Cheers

@facebook-github-bot
Copy link
Contributor

@PeteTheHeat has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mikehardy added a commit to invertase/notifee that referenced this pull request Jun 14, 2021
- note that typedoc is *not* updated past v19, it requires a forward-port
- de-lint all the files

Note that a forced resolution of newer versions of the eslint/typescript integration were necessary, see:
facebook/react-native#31490
mikehardy added a commit to invertase/notifee that referenced this pull request Jun 14, 2021
- cavy patch no longer needed, upstream integrated change via PR from here
- note that typedoc is *not* updated past v19, it requires a forward-port
- de-lint all the files

Note that a forced resolution of newer versions of the eslint/typescript integration were necessary, see:
facebook/react-native#31490
@mikehardy
Copy link
Contributor

With apologies again for a direct ping, but @matt-oakes it appears based on a recent PR that you have npm publish powers and published this last time? I'll quickly admit I'm just reading old stuff and pinging people, which means this might not be correct and thus might be annoying, apologies in advance if that's the case

@PeteTheHeat
Copy link
Contributor

Are the failures to test_ios_unit_hermes relevant to the changes in this diff?

@mikehardy
Copy link
Contributor

@PeteTheHeat unfortunately I cannot see in CircleCI as it requires a login and to do so via github would allow it read/write access to all sorts of repos I share with others, I can't do that.

However, the change here is a bump to the transitive dependencies used by linters, specifically the combo of eslint and typescript. That should fail a lint type check, not a unit test check. Specifically I would presume "Facebook Internal - Linter" or "ci/circleci:analyze_code" to be the ones affected?

Further, given there is another run named "ci/circleci: test_ios_unit_jsc" that passes, and only the hermes "ci/circleci: test_ios_unit_hermes" variant is failing, I would assume (danger, danger) that this could not have anything to do with the failure.

Software always surprises, bit I think it's unrelated.

@matt-oakes
Copy link
Contributor

@mikehardy I did have publish access before and, as far as I'm aware, I still do. However, I don't have permission to merge anything so it would need to be reviewed and merged before I could publish it.

@mikehardy
Copy link
Contributor

Ah, I see @matt-oakes - I thought that was what the import from the facebook bot meant, but I suppose we need to wait for @PeteTheHeat to do something else perhaps pending resolution of unit tests on circleci which now appear to have failed on both jsc and hermes variant (which is odd, since jsc was green before?). But I have no insight there so 🤷 not sure what else I can do to get the dependencies bump other than continue with the resolutions block I've got in package.json locally and hope.

@facebook-github-bot
Copy link
Contributor

@PeteTheHeat merged this pull request in 3b751d3.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 15, 2021
@matt-oakes
Copy link
Contributor

@mikehardy @PeteTheHeat Sorry, I should have said before, but ideally a change should be merged in that bumps the version number of the package so the version that's in the repo stays in sync with what's published.

Can someone open up a PR to do that? I presume it's a major version bump because of the bump in dependencies?

mikehardy added a commit to mikehardy/react-native that referenced this pull request Jun 16, 2021
facebook#31490 bumped the dependency versions of `@typescript-eslint` project across a semver major,
bumping the package version here a semver major to correspond, prior to publishing
@mikehardy
Copy link
Contributor

@matt-oakes oh of course 🤦 - just posted #31372

facebook-github-bot pushed a commit that referenced this pull request Jun 16, 2021
Summary:
#31490 bumped the dependency versions of `typescript-eslint` project across a semver major,
bumping the package version here a semver major to correspond, prior to publishing

This is being done [in concert with](#31490 (comment)) matt-oakes who may hopefully publish the result and PeteTheHeat
who merged #31490 (thanks!)

I copied the changelog chunk from the last version bump merged here, hopefully it is correct + useful

## Changelog

[General] [Fixed] - Upgrade dependencies / version of eslint package

Pull Request resolved: #31732

Test Plan: Run in local project

Reviewed By: TheSavior

Differential Revision: D29167289

Pulled By: PeteTheHeat

fbshipit-source-id: 464365b2bff936ad5a33f9a39eb8e56aed8e8715
@matt-oakes
Copy link
Contributor

@mikehardy @wcandillon Published as v3.0.0.

@mikehardy
Copy link
Contributor

Fantastic! I really appreciate the collaboration getting this in the tree and out on npmjs, thanks @PeteTheHeat and @matt-oakes and @wcandillon for posting it originally

@Bibazavr
Copy link

I'm upset that my similar merge was just ignored

@mikehardy
Copy link
Contributor

@Bibazavr oh I didn't even see that when I searched! Don't be too upset though. In general I think react-native maintenance at the core level is a little bit reactive and definitely we're all busier than we should be. I think I was just the "squeaky wheel" on this one and it worked 🤷 . Keep trying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants