Upgrade ESLint to 6 #236

kylemh · 2019-08-19T22:19:22Z

Resolves #222

Summary

For those uninterested in seeing the conversation below, we simply upgraded the dependency and removed an assertion regarding the number of times fs.readFileSync is called, as it is an integration detail that ESLint controls - not this library.

Original Post

So... logging the calls of fsMock.readFileSync yields:

The following test logs error if it cannot read the file... sets and resets the mock at the test-level, instead of the test lifecycle; however, when I attempt this workaround...

test('reads text from fs if filePath is provided but not text', () => {
  const originalMock = fsMock.readFileSync;
  fsMock.readFileSync = jest.fn();

  const filePath = '/blah-blah/some-file.js';
  format({ filePath });

  expect(fsMock.readFileSync).toHaveBeenCalledWith(filePath, 'utf8');

  // one hit to get the file and one for the eslintignore
  expect(fsMock.readFileSync).toHaveBeenCalledTimes(2);

  fsMock.readFileSync = originalMock;
});

I'm met with many other broken tests:

I skimmed the changelog for ESLint since v5, but couldn't find a reasoning for the increased calls; so I'm left to imagine that

a) It is invalid and the way the tests are written currently need a massive overhaul to avoid leaking into other . tests.

OR

b) It is valid, but I'm unsure how to document the extra calls.

codecov-io · 2019-08-19T22:23:24Z

Codecov Report

Merging #236 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #236   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         208    208           
  Branches       42     42           
=====================================
  Hits          208    208

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c87b69b...acaa3b5. Read the comment docs.

kylemh · 2019-09-20T21:56:20Z

@robertpaul01 I feel like you didn’t read anything.

No it can’t be merged.

jakeleventhal · 2019-09-20T22:07:01Z

@kylemh lmfao

kylemh · 2019-09-22T01:49:11Z

Hell yeah, well why didn't you say so!?

Also, you debugged the test incorrectly. You need to do this to not crash all the test like you did because you stubbed the readFileSync to an empty function.

This is totally correct - didn't realize fs was mocked separately as part of the Jest setup, so I needed to pass it the mock implementation, or - better yet - I'll just spy on the original mock fn implementation.

Unfortunately, even with the corrections, I am still seeing 5 function calls versus 2. To be clear: I tried both the done fn implementation you showed and the spy method I tried.

The janky tests should be turned off or updated.

Doesn't seem like the safest strategy, which is why I tried opening up this issue for discussion. I agree with you and think these tests are "janky", but the library main contributors should be the one to see in finality.

numso · 2019-09-24T03:07:20Z

@kylemh: Based on what @robertpaul01 wrote, the way prettier-eslint interacts with eslint@6 causes .eslintignore to be read multiple times during the test. That test should now assert that readFileSync was called 5 times.

IMHO The test in question is testing more of the internal details of eslint than it should. That test is just trying to ensure that format, given a filePath, reads and uses the file at that filePath. That being the case, I believe the call count assertion (expect(readFileSyncMockSpy).toHaveBeenCalledTimes(5);) is unneeded and should be removed.

kylemh · 2019-09-24T06:31:35Z

Thank you all for making it a discussion y'all ❤️
I agree that the most important assertion is that readFileSync is called and with the right parameters, but not necessarily the number of calls.

@zimme if you feel asserting the number of calls is necessary let me know, and I'll add a comment to this pull request as contextual information. Otherwise, this PR should be ready.

zimme · 2019-09-24T06:50:21Z

I'll make sure to carve out some time this week for this so we can get a new major version out with support for eslint 6

JSanchezIO · 2019-09-30T16:12:50Z

Has any progress been made on this?

zimme · 2019-09-30T16:19:18Z

I started to look at this today, however family life intervened and here we are me hoping to get a few hours available this week to look at this and hopefully merge

kylemh · 2019-09-30T21:10:05Z

Clearly zimme will get to it when he can. No need to request for more updates.

tweet · 2019-10-24T15:39:41Z

@zimme Any update for this PR?

zimme · 2019-10-24T15:40:34Z

I will try my best to test and merge this within a week.

dimitropoulos · 2019-10-24T21:07:28Z

@zimme is there any way I can help?

blueandhack · 2019-11-02T12:23:23Z

I will try my best to test and merge this within a week.

@zimme Hi Simon, first I want to say, thank you! we are still waiting for you. I know this is hard that maintainers to maintain a popular public repository, but you know, a lot of people and repository waiting for this merge. Thank you again!

tomkel · 2019-11-04T02:04:44Z

Feel free to put up a donation link so everyone who wants it done can chip in to help

JSanchezIO · 2019-11-06T04:26:17Z

I'm willing to donate, but I don't think that should be the motivation for @zimme to pull the trigger on this. My frustration is that this PR is ready to be merged, and there isn't anything I can do to help. Is there no other course of action? Is someone else eligible to review and merge this?

kylemh · 2019-11-06T23:29:24Z

Just because it's ready to be merged doesn't mean it's ready to be used in everybody's application.

We had a discussion at the top of the PR on a point that we think we have a good understanding of now, but - as an author of a popular open source library - the hesitation becomes around not wanting to bump versions before they're as certain as possible.

If this ships and actually leads to bugs, now the author has created their own stress to correct everything ASAP. If they don't, issues increase and notifications blow up increasing more stress.

Please stop bumping this thread. They'll release it when they release it.

pelotom · 2019-11-06T23:32:49Z

Why not just release under a beta tag and let early adopters find the bugs for you? Seems like everyone's happy then...

kylemh · 2020-01-05T19:42:39Z

@zimme care to release a beta tag with this merged? I can subscribe to the repo and pay attention to issues. If we get any from the beta, I can make the reversion PR and ping you.

jamesblack · 2020-01-06T21:14:46Z

Clearly zimme will get to it when he can. No need to request for more updates.

Unfortnately, React Native 0.60.0 specifies this version of ESLint as a requirement. (0.62.0-rc1 removes the requirement.) Note that `prettier-eslint` is not yet fully compatible with ESLint 6, although an open PR exists which should remedy this: prettier/prettier-eslint#236

DarkMikey · 2020-03-27T07:56:17Z

Sorry to ask, but what is the workaround to get prettier to work with eslint 6?

kylemh · 2020-03-27T16:43:42Z

Assuming you install the relevant dependencies, you’ll want a lint config like this:

module.exports = {
  env: {
    browser: true,
    commonjs: true,
    es6: true,
    node: true,
  },

  extends: [
    'prettier',
    'prettier/react',
  ],

  parserOptions: {
    ecmaFeatures: {
      jsx: true,
    },
    ecmaVersion: 2018,
    sourceType: 'module',
  },

  plugins: [
    'prettier',
  ],

  rules: {
    // Prettier Plugin Rules
    'prettier/prettier': 'error',
  },
};

codecov-commenter · 2020-05-21T19:37:21Z

Codecov Report

Merging #236 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #236   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          208       208           
  Branches        42        42           
=========================================
  Hits           208       208

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71f8d84...62f85fb. Read the comment docs.

kylemh · 2020-05-21T19:37:44Z

Ready to merge and tag at your discretion @hamzahamidi

hamzahamidi · 2020-05-21T20:10:43Z

@kylemh it appears the pr no longer passes. I'll take a look

hamzahamidi · 2020-05-21T20:17:38Z

@kylemh The checks pass now

hamzahamidi · 2020-05-21T20:25:06Z

I'll merge this branch as a breakingChange: Eslint 6+

zimme · 2020-05-21T20:31:07Z

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

numso · 2020-05-21T20:38:41Z

@hamzahamidi FYI looks like things may have gotten lost in between PRs getting merged and reverted. It looks like master is still pointing to eslint@5

prettier-eslint/package.json

Line 25 in b2ae087

"eslint": "^5.0.0",

kylemh · 2020-05-21T20:51:49Z

Strange. It was here: 95169c5

hamzahamidi · 2020-05-21T20:53:03Z

Ok I'll ready another pr.

hamzahamidi · 2020-05-21T21:03:12Z

#337

Kyle Holmberg added 2 commits August 19, 2019 15:14

Upgrade ESLint

8c61b5f

Adjust test

0d6b33f

kylemh changed the title ~~Upgrade eslint to 6~~ Upgrade ESLint to 6 Aug 19, 2019

kylemh mentioned this pull request Aug 19, 2019

Not working properly width eslint 6 #222

Closed

tnrich mentioned this pull request Aug 21, 2019

Support having plugins as dependencies in shareable config eslint/eslint#3458

Closed

ntotten mentioned this pull request Aug 23, 2019

[BUG] not working with eslint 6 prettier/prettier-vscode#870

Closed

Spy mock instead of re-implementing another mock

13a4556

Remove call count assertion from test

c1d70a6

Upgrade to eslint@6.4.0

acaa3b5

svipas mentioned this pull request Sep 24, 2019

Does it support eslint 6? svipas/vscode-prettier-plus#4

Closed

BPScott mentioned this pull request Oct 26, 2019

Sewing-kit bump Shopify/polaris#2369

Merged

hamzahamidi approved these changes May 21, 2020

View reviewed changes

Merge branch 'master' into upgrade-eslint-to-6

95169c5

Merge branch 'master' into upgrade-eslint-to-6

62f85fb

hamzahamidi merged commit b2ae087 into prettier:master May 21, 2020

kylemh deleted the upgrade-eslint-to-6 branch May 21, 2020 20:30

zimme added the released label May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade ESLint to 6 #236

Upgrade ESLint to 6 #236

kylemh commented Aug 19, 2019 •

edited

codecov-io commented Aug 19, 2019 •

edited

kylemh commented Sep 20, 2019

jakeleventhal commented Sep 20, 2019

kylemh commented Sep 22, 2019 •

edited

numso commented Sep 24, 2019

kylemh commented Sep 24, 2019

zimme commented Sep 24, 2019

JSanchezIO commented Sep 30, 2019

zimme commented Sep 30, 2019

kylemh commented Sep 30, 2019

tweet commented Oct 24, 2019

zimme commented Oct 24, 2019

dimitropoulos commented Oct 24, 2019

blueandhack commented Nov 2, 2019 •

edited

tomkel commented Nov 4, 2019 •

edited

JSanchezIO commented Nov 6, 2019

kylemh commented Nov 6, 2019

pelotom commented Nov 6, 2019

kylemh commented Jan 5, 2020

jamesblack commented Jan 6, 2020

DarkMikey commented Mar 27, 2020

kylemh commented Mar 27, 2020 •

edited

codecov-commenter commented May 21, 2020 •

edited

kylemh commented May 21, 2020

hamzahamidi commented May 21, 2020

hamzahamidi commented May 21, 2020

hamzahamidi commented May 21, 2020 •

edited

zimme commented May 21, 2020

numso commented May 21, 2020

kylemh commented May 21, 2020

hamzahamidi commented May 21, 2020

hamzahamidi commented May 21, 2020

Upgrade ESLint to 6 #236

Upgrade ESLint to 6 #236

Conversation

kylemh commented Aug 19, 2019 • edited

Summary

Original Post

codecov-io commented Aug 19, 2019 • edited

Codecov Report

kylemh commented Sep 20, 2019

jakeleventhal commented Sep 20, 2019

kylemh commented Sep 22, 2019 • edited

numso commented Sep 24, 2019

kylemh commented Sep 24, 2019

zimme commented Sep 24, 2019

JSanchezIO commented Sep 30, 2019

zimme commented Sep 30, 2019

kylemh commented Sep 30, 2019

tweet commented Oct 24, 2019

zimme commented Oct 24, 2019

dimitropoulos commented Oct 24, 2019

blueandhack commented Nov 2, 2019 • edited

tomkel commented Nov 4, 2019 • edited

JSanchezIO commented Nov 6, 2019

kylemh commented Nov 6, 2019

pelotom commented Nov 6, 2019

kylemh commented Jan 5, 2020

jamesblack commented Jan 6, 2020

DarkMikey commented Mar 27, 2020

kylemh commented Mar 27, 2020 • edited

codecov-commenter commented May 21, 2020 • edited

Codecov Report

kylemh commented May 21, 2020

hamzahamidi commented May 21, 2020

hamzahamidi commented May 21, 2020

hamzahamidi commented May 21, 2020 • edited

zimme commented May 21, 2020

numso commented May 21, 2020

kylemh commented May 21, 2020

hamzahamidi commented May 21, 2020

hamzahamidi commented May 21, 2020

kylemh commented Aug 19, 2019 •

edited

codecov-io commented Aug 19, 2019 •

edited

kylemh commented Sep 22, 2019 •

edited

blueandhack commented Nov 2, 2019 •

edited

tomkel commented Nov 4, 2019 •

edited

kylemh commented Mar 27, 2020 •

edited

codecov-commenter commented May 21, 2020 •

edited

hamzahamidi commented May 21, 2020 •

edited