Upgrade: update dependencies after dropping support for Node <8 #53
Conversation
| @@ -93,7 +93,7 @@ function updateDeeply(target, override) { | |||
| } | |||
|
|
|||
| for (const key in override) { | |||
| if (override.hasOwnProperty(key)) { | |||
| if (Object.prototype.hasOwnProperty.call(override, key)) { | |||
There was a problem hiding this comment.
96:22 error Do not access Object.prototype method 'hasOwnProperty' from target object no-prototype-builtins
This was manually fixed.
| }); | ||
| } | ||
|
|
||
| visitPattern(node, options, callback) { | ||
| let visitPatternOptions = options; |
There was a problem hiding this comment.
160:13 error Assignment to function parameter 'callback' no-param-reassign
161:13 error Assignment to function parameter 'options' no-param-reassign
This was manually fixed.
| @@ -122,7 +122,7 @@ describe("export declaration", () => { | |||
| }); | |||
|
|
|||
| it("should refer exported references#1", () => { | |||
| const ast = espree("export {x};"); | |||
| const ast = espree("const x = 1; export {x};"); | |||
There was a problem hiding this comment.
More recent versions of Espree will throw an error if the export is not defined: SyntaxError: Export 'x' is not defined
| }); | ||
|
|
||
| it("should refer exported references#2", () => { | ||
| const ast = espree("export {v as x};"); | ||
| const ast = espree("const v = 1; export {v as x};"); |
There was a problem hiding this comment.
More recent versions of Espree will throw an error if the export is not defined: SyntaxError: Export 'v' is not defined
| @@ -29,7 +29,7 @@ const analyze = require("..").analyze; | |||
| describe("ES6 super", () => { | |||
| it("is not handled as reference", () => { | |||
| const ast = espree(` | |||
| class Hello { | |||
| class Foo extends Bar { | |||
There was a problem hiding this comment.
More recent versions of Espree will throw an error when super methods are called in methods of a class that is not a subclass: SyntaxError: super() call outside constructor of a subclass
| @@ -31,39 +31,22 @@ describe("typescript", () => { | |||
|
|
|||
| const scopeManager = analyze(ast); | |||
|
|
|||
| expect(scopeManager.scopes).to.have.length(4); | |||
| expect(scopeManager.scopes).to.have.length(2); | |||
There was a problem hiding this comment.
Does this seem correct? I think this makes sense, since the first two are type definitions and so shouldn't actually create a new scope? Would love it if @bradzacher or @JamesHenry would be so kind to chime in!
There was a problem hiding this comment.
Yes. I think that eslint/typescript-eslint-parser#540 has affected it.
|
@platinumazure Thanks for the review! Changes made - ready for re-review :) |
kaicataldo
changed the title
This PR updates some outdated dependencies. I have also run the autofixer and will comment on places I manually changed things.
I haven't worked in this repository very much, so I'd really love feedback/reviews from others! I'm specifically unsure of the TypeScript change (though I think what I have here makes sense?).
CI is failing in Node <=6 because we've dropped support in ESLint v6. Should we also drop support and do a major release ofeslint-scope?Edit: PR can be found here.Edit 2: As mentioned in #54, Webpack relies on this package directly and still supports Node@6. Maybe we want to keep support for Node 6 (and just drop support for Node 4)? We won't be able to upgrade to the latest version of ESLint in our devDependencies, though.I've downgraded ESLint to v5 and it now passes CI in Node 6, however this still fails in Node 4. At this point, I guess we should just wait until we drop Node <8.Edit 3: The TSC resolved to drop Node <8 for this package. See #54.