[New]: Provide both range and start & end property on Node, support eslint v7 #97

MichaelDeBoey · 2020-05-09T11:59:31Z

(dev)Dependencies should be compatible with ESLint 7 too before we can merge this one:

eslint-config-airbnb-base (eslint v7 support airbnb/javascript#2223)
- [eslint config] [*] [new] add eslint v7 airbnb/javascript#2240
- Release

Closes #96

…onfig-airbnb-base`, `eslint-plugin-import`, `flow-parser`

coveralls · 2020-05-09T13:09:32Z

Coverage increased (+0.03%) to 98.537% when pulling e2665f6 on MichaelDeBoey:eslint-7 into ef18995 on evcohen:master.

MichaelDeBoey · 2020-05-09T13:19:13Z

@evcohen @ljharb CI is passing here, but supporting ESLint 7.x in eslint-plugin-jsx-a11y fails CI and throws the following error when using getPropValue & getLiteralPropValue :

 Use node.range[0] instead of node.start

Can't figure out what causes this error though... 🤔
The only time I find node.start usage is in getProp-parse-test:
https://github.com/evcohen/jsx-ast-utils/blob/ef189952cf16c126202e0524565d8d5787908d20/__tests__/src/getProp-parser-test.js#L103

If any of you can point me in the right direction, that would be great! 🙂

MichaelDeBoey · 2020-05-09T14:03:08Z

I've been doing some research and found out the reason why they disallowed the usage node.start & node.end.

After disabling the return of both node.start & node.end, CI is failing on this side because @babel/parser is still returning them.
So I think this should be fixed on their side too I guess? 🤔

Another option would be to change the deepStrictEqual assert to a normal deepEqual, but that can cause other bugs I'm afraid.

MichaelDeBoey · 2020-05-09T15:32:16Z

Giving this some thoughts and I think we should fix it on our end if we want to keep supporting Node@<6.x.
Even when @babel/parser fixes this, babylon will still keep failing CI then.

ljharb · 2020-05-09T16:40:34Z

Since this package only uses eslint as a dev dep, this PR isn't a feature; and it's blocked on the plugins it uses supporting eslint v7.

We definitely need to keep supporting the same node versions. In this case, we should find a way to configure the tests to use start/end only when it's allowed by RuleTester.

MichaelDeBoey · 2020-05-09T17:02:51Z

@evcohen @ljharb I've managed to change the code so that it doesn't return the non-standard node.start & node.end anymore.

Since it's non-standard and estree & @typescript-eslint/parser have already removed it completely, I think we should do too.

MichaelDeBoey · 2020-05-09T18:51:25Z

@evcohen @ljharb All tests are green 🙂

ljharb

This looks good, but it will be a breaking change due to removing the start and end properties. Is there any way you can think of to avoid making that be the case?

MichaelDeBoey · 2020-05-09T23:28:09Z

@ljharb That will indeed be a breaking change.
Can't think of a way to do this without making this a breaking change though 😕

This will only make jsx-ast-utils a breaking change, so eslint-plugin-jsx-a11y won't have to be a breaking one.

ljharb · 2020-05-10T17:51:23Z

I don’t think it’s worth a breaking change to any package just so it can use a newer version of the linter - leaving this package on eslint 6 for the foreseeable future seems fine to me.

MichaelDeBoey · 2020-05-10T18:01:03Z

@ljharb That will break all ESLint plugins/configs (like eslint-plugin-jsx-a11y) that use this package if they want to support ESLint 7.
Those plugins/configs can cause other plugins or configs to fail supporting ESLint 7 too.

ljharb · 2020-05-10T20:58:25Z

I'm not sure why it would? it's RuleTester that can't use start/end, not plugins.

MichaelDeBoey · 2020-05-10T23:07:30Z

It's because node.start/node.end are non-standard and are mostly wrongly used.
They should never been used or been public.
That's why they're now deprecated.

RuleTester comes from the eslint package btw, so it's officially a deprecation error.

ljharb · 2020-05-11T02:48:32Z

Sure - but error or not, consumers will still work, no?

MichaelDeBoey · 2020-05-11T08:18:10Z

@ljharb Since it's a hard deprecation that's throwing an error, It won't.

ljharb · 2020-05-16T20:13:12Z

It throws an error in RuleTester - it wouldn't throw one at runtime, since eslint itself doesn't interact with the code between this package and rules.

MichaelDeBoey · 2020-05-17T00:07:27Z

@ljharb Any suggestion on how we can fix this then? 🤔

ljharb · 2020-05-17T06:57:04Z

I think we could revert the runtime removal of start/end, and then in the tests, skip the appropriate tests when eslint is 7+?

MichaelDeBoey · 2020-05-17T09:12:43Z

@ljharb If we can do this somehow, that will still fail tests in our dependants, no? 🤔
Those will have to use the same trick all over again then, which is not really what they would want.

ESLint is pushing towards not using .start and .end anymore and will probably fully deprecate/remove them from espree in the future.
I don’t get why they didn’t do that right away though.

ESLint/Espree is not a dependency, but it’s some sort of “hidden” peerDependency in my opinion, since this package relies on the AST format they provide.
So I think a breaking change for this package is justified, since people who don’t want or can’t update (yet), can still use the current version.

People who need to get rid of the .start and .end errors and don’t want to implement the whole trick you’re suggesting all over again can upgrade to the new version.

ljharb · 2020-05-17T15:15:59Z

That’s fine; dependents can change themselves to use the range property when using eslint 7. It wouldn’t fail our dependents using eslint 6.

In other words, because this package does not require eslint 7 - it works on both 6 and 7 - and start and end work fine in v6 - this package should continue to provide them.

MichaelDeBoey · 2020-05-17T15:22:52Z

@ljharb That's the whole point: this package wouldn't work with dependents who want to use ESLint 7.x at this moment (see https://github.com/evcohen/eslint-plugin-jsx-a11y/pull/684), because this package is using node.start, node.end.

eslint-plugin-jsx-a11y is using this package and this package is using node.start & node.end, so eslint-plugin-jsx-a11y can't use this package anymore when supporting ESLint 7 if we don't change anything here.

ljharb · 2020-05-17T20:41:22Z

Thanks for bearing with me, I see the test failure here.

So, there's two considerations for back compat:

this package should be able to read start/end in eslint < 7, just in case. This is pretty easy with something like const [start, end] = node.range || [node.start, node.end].
any objects this package returns to callers should have both range and start/end - this shouldn't matter, since the callers aren't eslint 7.

Does that sound workable?

MichaelDeBoey · 2020-05-18T19:36:16Z

@ljharb I just logged out start, end & range in getBaseProps and run tests.

https://github.com/evcohen/jsx-ast-utils/blob/0964cf5471b8157d9f60d4ccf4ca574f98ecfd9c/src/getProp.js#L51-L63

It seems like either range or start & end is passed:

    console.log src/getProp.js:57
      { range: [ 11, 13 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: [ 11, 20 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: undefined, start: 11, end: 13 }
    console.log src/getProp.js:57
      { range: undefined, start: 11, end: 20 }
    console.log src/getProp.js:57
      { range: [ 11, 13 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: [ 11, 13 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: [ 11, 13 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: undefined, start: 11, end: 13 }
    console.log src/getProp.js:57
      { range: undefined, start: 11, end: 13 }
    console.log src/getProp.js:57
      { range: undefined, start: 11, end: 13 }
    console.log src/getProp.js:57
      { range: [ 11, 13 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: [ 15, 29 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: [ 11, 29 ], start: undefined, end: undefined }
    console.log src/getProp.js:57
      { range: undefined, start: 11, end: 13 }
    console.log src/getProp.js:57
      { range: undefined, start: 15, end: 29 }
    console.log src/getProp.js:57
      { range: undefined, start: 11, end: 29 }

So tests will fail if I change the code to your suggestion

function getBaseProps({
  loc,
  range,
  ...node
}) {
  const [start, end] = range || [node.start, node.end];

  return {
    // ...
  };
}

ljharb · 2020-05-18T19:38:23Z

Then it'd be semver-minor to pass both; could we do that?

MichaelDeBoey · 2020-05-18T19:40:53Z

Then we'll have the same as removing them, except we're adding the other missing ones, no? 🤔

So both doing it in code + in the tests?

ljharb · 2020-05-18T19:53:13Z

What I mean is, this package at runtime can provide both forms, by computing the missing ones from the present ones. The tests will use "range" when present, else "start/end".

MichaelDeBoey · 2020-05-18T20:01:42Z

So what you're saying is doing it like this?

function getBaseProps({
  loc,
  range,
  ...node
}) {
  const [start, end] = range || [node.start, node.end];

  return {
    end,
    loc: getBaseLocation(loc),
    range: [start, end],
    start,
  };
}

This will create range on Nodes that don't have a range and where it's not logical to have a range I think.

    Expected value to deeply and strictly equal to:
      {"end": 24, "loc": {"end": {"column": 24, "line": 1}, "filename": "test.js", "start": {"column": 5, "line": 1}}, "name": {"end": 7, "loc": {"end": {"column": 7, "line": 1}, "filename": "test.js", "start": {"column": 5, "line": 1}}, "name": "id", "start": 5, "type": "JSXIdentifier"}, "start": 5, "type": "JSXAttribute", "value": {"end": 24, "expression": {"end": 23, "expressions": [{"end": 18, "loc": {"end": {"column": 18, "line": 1}, "filename": "test.js", "identifierName": "bar", "start": {"column": 15, "line": 1}}, "name": "bar", "start": 15, "type": "Identifier"}], "loc": {"end": {"column": 23, "line": 1}, "filename": "test.js", "start": {"column": 9, "line": 1}}, "quasis": [{"end": 13, "loc": {"end": {"column": 13, "line": 1}, "filename": "test.js", "start": {"column": 10, "line": 1}}, "start": 10, "tail": false, "type": "TemplateElement", "value": {"cooked": "foo", "raw": "foo"}}, {"end": 22, "loc": {"end": {"column": 22, "line": 1}, "filename": "test.js", "start": {"column": 19, "line": 1}}, "start": 19, "tail": true, "type": "TemplateElement", "value": {"cooked": "baz", "raw": "baz"}}], "start": 9, "type": "TemplateLiteral"}, "loc": {"end": {"column": 24, "line": 1}, "filename": "test.js", "start": {"column": 8, "line": 1}}, "start": 8, "type": "JSXExpressionContainer"}}
    Received:
      {"end": 24, "loc": {"end": {"column": 24, "line": 1}, "filename": "test.js", "start": {"column": 5, "line": 1}}, "name": {"end": 7, "loc": {"end": {"column": 7, "line": 1}, "filename": "test.js", "start": {"column": 5, "line": 1}}, "name": "id", "range": [5, 7], "start": 5, "type": "JSXIdentifier"}, "range": [5, 24], "start": 5, "type": "JSXAttribute", "value": {"end": 24, "expression": {"end": 23, "expressions": [{"end": 18, "loc": {"end": {"column": 18, "line": 1}, "filename": "test.js", "identifierName": "bar", "start": {"column": 15, "line": 1}}, "name": "bar", "start": 15, "type": "Identifier"}], "loc": {"end": {"column": 23, "line": 1}, "filename": "test.js", "start": {"column": 9, "line": 1}}, "quasis": [{"end": 13, "loc": {"end": {"column": 13, "line": 1}, "filename": "test.js", "start": {"column": 10, "line": 1}}, "start": 10, "tail": false, "type": "TemplateElement", "value": {"cooked": "foo", "raw": "foo"}}, {"end": 22, "loc": {"end": {"column": 22, "line": 1}, "filename": "test.js", "start": {"column": 19, "line": 1}}, "start": 19, "tail": true, "type": "TemplateElement", "value": {"cooked": "baz", "raw": "baz"}}], "start": 9, "type": "TemplateLiteral"}, "loc": {"end": {"column": 24, "line": 1}, "filename": "test.js", "start": {"column": 8, "line": 1}}, "range": [8, 24], "start": 8, "type": "JSXExpressionContainer"}}

ljharb · 2020-05-18T20:06:10Z

Not unconditionally ofc; where one of range or start+end is present, i would expect both to be.

MichaelDeBoey · 2020-05-18T20:35:31Z

Did some investigation and I came to the following conclusions:

All tests in getProp-parser-test that are using the flow parser aren't passing start & end
All tests in getProp-parser-test that are using the babel parser aren't passing range
All tests in getProp-test (that are all using the babel parser) aren't passing range either

This means that the user of jsx-ast-utils is already getting different output according to the parser they're using, so I think we should indeed provide both range and start & end for now then.

The ultimate goal would be to remove start & end from the output imo though.

ljharb · 2020-05-18T20:53:00Z

Totally agree; i'd love to ship a non-major update for this, and then a major update that does nothing except drop start/end and some older eslint versions.

MichaelDeBoey · 2020-05-18T20:54:25Z

On it boss 😉

…slint v7 Co-authored-by: Michaël De Boey <[email protected]> Co-authored-by: Jordan Harband <[email protected]>

MichaelDeBoey · 2020-05-18T21:56:28Z

@ljharb Fixed like we said & all green 🎉

ljharb · 2020-05-25T03:15:33Z

so, i think this really can't land yet. This repo uses eslint-config-airbnb-base, which does not yet support eslint 7. so, that has to be published first (which means eslint-plugin-import has to be published first).

I'll rebase and land this once that's done.

epmatsw · 2020-06-08T15:54:02Z

Looks like eslint-plugin-import just published v7 support in ^2.21.0!

MichaelDeBoey · 2020-06-10T19:26:20Z

Ping @ljharb

ljharb

This LGTM, and seems like it's a semver-minor.

__tests__/src/getProp-parser-test.js

MichaelDeBoey · 2020-06-10T21:58:14Z

@ljharb Resolved all and all tests are green

ljharb · 2020-06-10T22:04:48Z

That's just because tests don't run npm ls tho; eslint-config-airbnb-base is still incompatible with eslint 7.

MichaelDeBoey · 2020-06-10T22:22:33Z

@ljharb Is there anything I can do to help ESLint 7.x compatibility land on eslint-config-airbnb-base?

ljharb · 2020-06-10T22:24:45Z

Nope, it's on my list, I just need to get to it.

MichaelDeBoey · 2020-06-11T08:37:15Z

@ljharb I've updated eslint-config-airbnb-base, so I think this one can be merged, no? 🤔

Or is there something else I should do first?

[Dev Deps] update @babel/core, @babel/parser, eslint, `eslint-c…

f6a440c

…onfig-airbnb-base`, `eslint-plugin-import`, `flow-parser`

MichaelDeBoey mentioned this pull request May 9, 2020

feat: Support ESLint 7.x jsx-eslint/eslint-plugin-jsx-a11y#684

Merged

12 tasks

ljharb approved these changes May 9, 2020

View reviewed changes

MichaelDeBoey requested a review from ljharb May 16, 2020 12:09

[New]: Provide both range and start & end property on Node, support e…

5194d7f

…slint v7 Co-authored-by: Michaël De Boey <[email protected]> Co-authored-by: Jordan Harband <[email protected]>

ljharb self-assigned this May 25, 2020

ljharb approved these changes Jun 10, 2020

View reviewed changes

__tests__/src/getProp-parser-test.js Outdated Show resolved Hide resolved

MichaelDeBoey changed the title ~~feat: Support ESLint 7.x~~ feat: Provide both range and start & end property on Node Jun 10, 2020

MichaelDeBoey requested a review from ljharb June 10, 2020 22:04

MichaelDeBoey mentioned this pull request Jun 10, 2020

[Breaking]: Don't return node.start & node.end #100

Merged

ljharb mentioned this pull request Jun 11, 2020

eslint v7 support airbnb/javascript#2223

Closed

19 tasks

ljharb changed the title ~~feat: Provide both range and start & end property on Node~~ [New]: Provide both range and start & end property on Node, support eslint v7 Jun 11, 2020

ljharb added the enhancement label Jun 11, 2020

ljharb approved these changes Jun 11, 2020

View reviewed changes

ljharb merged commit 5194d7f into jsx-eslint:master Jun 11, 2020

MichaelDeBoey deleted the eslint-7 branch June 12, 2020 00:04

MichaelDeBoey mentioned this pull request Jun 12, 2020

Support ESLint 7.x #96

Closed

[New]: Provide both range and start & end property on Node, support eslint v7 #97

[New]: Provide both range and start & end property on Node, support eslint v7 #97

Conversation

MichaelDeBoey commented May 9, 2020 • edited Loading

coveralls commented May 9, 2020 • edited Loading

MichaelDeBoey commented May 9, 2020 • edited Loading

MichaelDeBoey commented May 9, 2020

MichaelDeBoey commented May 9, 2020

ljharb commented May 9, 2020

MichaelDeBoey commented May 9, 2020 • edited Loading

MichaelDeBoey commented May 9, 2020

ljharb left a comment

Choose a reason for hiding this comment

MichaelDeBoey commented May 9, 2020 • edited Loading

ljharb commented May 10, 2020

MichaelDeBoey commented May 10, 2020

ljharb commented May 10, 2020

MichaelDeBoey commented May 10, 2020

ljharb commented May 11, 2020

MichaelDeBoey commented May 11, 2020

ljharb commented May 16, 2020

MichaelDeBoey commented May 17, 2020 • edited Loading

ljharb commented May 17, 2020

MichaelDeBoey commented May 17, 2020 • edited Loading

ljharb commented May 17, 2020

MichaelDeBoey commented May 17, 2020 • edited Loading

ljharb commented May 17, 2020

MichaelDeBoey commented May 18, 2020

ljharb commented May 18, 2020

MichaelDeBoey commented May 18, 2020

ljharb commented May 18, 2020

MichaelDeBoey commented May 18, 2020

ljharb commented May 18, 2020

MichaelDeBoey commented May 18, 2020

ljharb commented May 18, 2020

MichaelDeBoey commented May 18, 2020

MichaelDeBoey commented May 18, 2020

ljharb commented May 25, 2020 • edited Loading

epmatsw commented Jun 8, 2020

MichaelDeBoey commented Jun 10, 2020

ljharb left a comment

Choose a reason for hiding this comment

MichaelDeBoey commented Jun 10, 2020

ljharb commented Jun 10, 2020

MichaelDeBoey commented Jun 10, 2020

ljharb commented Jun 10, 2020

MichaelDeBoey commented Jun 11, 2020

MichaelDeBoey commented May 9, 2020 •

edited

Loading

coveralls commented May 9, 2020 •

edited

Loading

MichaelDeBoey commented May 9, 2020 •

edited

Loading

MichaelDeBoey commented May 9, 2020 •

edited

Loading

MichaelDeBoey commented May 9, 2020 •

edited

Loading

MichaelDeBoey commented May 17, 2020 •

edited

Loading

MichaelDeBoey commented May 17, 2020 •

edited

Loading

MichaelDeBoey commented May 17, 2020 •

edited

Loading

ljharb commented May 25, 2020 •

edited

Loading