Skip to content

⬆️ Upgrade ESLint TS parser and plugin#31490

Closed
wcandillon wants to merge 1 commit into
facebook:masterfrom
wcandillon:eslint-ts-upgrade
Closed

⬆️ Upgrade ESLint TS parser and plugin#31490
wcandillon wants to merge 1 commit into
facebook:masterfrom
wcandillon:eslint-ts-upgrade

Conversation

@wcandillon

Copy link
Copy Markdown
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

pull-bot commented May 7, 2021

Copy link
Copy Markdown
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.

DetailsCATEGORY 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
Copy Markdown
Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 1e39d8b

@analysis-bot

analysis-bot commented May 7, 2021

Copy link
Copy Markdown
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

@mikehardy mikehardy left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@mikehardy

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@mikehardy @wcandillon Published as v3.0.0.

@mikehardy

Copy link
Copy Markdown
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
Copy Markdown

I'm upset that my similar merge was just ignored

@mikehardy

Copy link
Copy Markdown
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