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

Add support to Typescript's node types #72

Merged
merged 18 commits into from
Apr 18, 2019
Merged

Add support to Typescript's node types #72

merged 18 commits into from
Apr 18, 2019

Conversation

vhfmag
Copy link
Contributor

@vhfmag vhfmag commented Feb 6, 2019

This PR:

  • Updates babel and related libraries to ^7 for Typescript support
  • Updates Jest to ^21 because updating babel broke tests
  • Replaces babylon with @babel/parserfor Typescript support
  • Implements support for Typescript's TSNonNullExpression and TSAsExpression node type and tests them

Fixes #71

@coveralls
Copy link

coveralls commented Feb 6, 2019

Coverage Status

Coverage increased (+1.1%) to 98.936% when pulling 7a610e8 on vhfmag:support-typescript-non-null-expression into b5debad on evcohen:master.

@vhfmag
Copy link
Contributor Author

vhfmag commented Feb 6, 2019

@ljharb Babel 7 dropped support for Node < 6. Maybe this should involve a major version bump?

@ljharb
Copy link
Member

ljharb commented Feb 6, 2019

@vhfmag it'd be better if we could instead change CI to always run babel in latest node LTS, and then switch to the desired node version for running tests.

@vhfmag
Copy link
Contributor Author

vhfmag commented Feb 6, 2019

@ljharb How would I do this? Generate an AST with @babel/parser, save it to a JSON and use it in the tests? I don't know how feasible is this, since there is not a single point where the AST is generated, but rather each test dynamically call the parser with some inline code.

@ljharb
Copy link
Member

ljharb commented Feb 6, 2019

oh no, not at all. I mean, nvm install --lts && npm run build && nvm use $actualVersion in travis.yml

@vhfmag
Copy link
Contributor Author

vhfmag commented Feb 6, 2019

Oh, understood. I tried transpiling both src and __tests__ folders with ["@babel/preset-env", { "targets": { "node": "4" } }] in Node 8 and running the transpiled tests with Node 4, but it still did not work. Probably something with @babel/parser, which is used in the tests. If retro-compatibility is a hard goal, maybe a bundling @babel/parser for Node 4 could solve it. Worth trying?

@ljharb
Copy link
Member

ljharb commented Feb 6, 2019

Babel 7 doesn't support node < 6; I don't think bundling. is going to be particularly great. It'd be nice if the workaround only lived in travis.yml.

@vhfmag
Copy link
Contributor Author

vhfmag commented Feb 6, 2019

I just don't see how to make it possible to run tests with Node 4, they fail because of the incompatibility of Node 4 😞

@ljharb
Copy link
Member

ljharb commented Feb 6, 2019

ah - if jest 21 is the one that dropped support for node 4, then we'd have to stick to jest 20.

The reality is that tape is the only test runner that reliably maintains support for older nodes, so eventually anything published will need to switch to it, but that's out of scope for this PR.

@vhfmag
Copy link
Contributor Author

vhfmag commented Feb 6, 2019

I just checked and Jest 21 supports Node 4 just fine. I meant that @babel/parser, used in the tests, does not support it. Maybe I could use either babylon or @babel/parser, according to which version of node is running.

@ljharb
Copy link
Member

ljharb commented Feb 6, 2019

ahhhhh gotcha. hmm. if there's a way to run the babel 6 parser in node 4, that'd be great - just as you say

@vhfmag
Copy link
Contributor Author

vhfmag commented Feb 7, 2019

@ljharb done! Everything is working again for Node >= 4. 😄

@ljharb ljharb requested a review from beefancohen February 7, 2019 16:08
__tests__/helper.js Outdated Show resolved Hide resolved
@kripod
Copy link

kripod commented Apr 18, 2019

I just submitted an issue to typescript-eslint and found out this pull request would resolve it. Are there any reasons against merging and releasing this TS-related improvement?

@ljharb
Copy link
Member

ljharb commented Apr 18, 2019

It needs to be rebased.

@vhfmag
Copy link
Contributor Author

vhfmag commented Apr 18, 2019

@ljharb done! I'll pinpoint the changes I've needed to do to make this easier to review.

// the throw expectation wrapper at that time.
// eslint-disable-next-line no-undef
expect(() => {
const runTest = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I have extracted the expected-to-fail function into a variable to conditionally expect it to fail or not according to Babel version

@ljharb ljharb merged commit 10400da into jsx-eslint:master Apr 18, 2019
@kripod
Copy link

kripod commented Apr 19, 2019

I would like to know whether it is possible to issue a new package release soon. This is a dealbreaker for TypeScript developers.

@beefancohen
Copy link
Contributor

Working on it now @kripod

@beefancohen
Copy link
Contributor

published v2.1.0

@kripod
Copy link

kripod commented Apr 20, 2019

Unfortunately, I'm still encountering the issue, even after upgrading to v2.1.0: typescript-eslint/typescript-eslint#437

$ eslint "**/*.{ts,tsx,js}"
Error: The prop value with an expression type of TSNonNullExpression could not be resolved. Please file issue to get this fixed immediately.
Occurred while linting C:\Development\Projects\gatsby-starter-strict\src\components\Layout.tsx:32
    at Object.extractLiteral [as JSXExpressionContainer] (C:\Development\Projects\gatsby-starter-strict\node_modules\jsx-ast-utils\lib\values\expressions\index.js:219:11)
    at getLiteralValue (C:\Development\Projects\gatsby-starter-strict\node_modules\jsx-ast-utils\lib\values\index.js:61:35)
    at extractValue (C:\Development\Projects\gatsby-starter-strict\node_modules\jsx-ast-utils\lib\getPropValue.js:24:12)
    at getLiteralPropValue (C:\Development\Projects\gatsby-starter-strict\node_modules\jsx-ast-utils\lib\getPropValue.js:56:10)
    at JSXAttribute (C:\Development\Projects\gatsby-starter-strict\node_modules\eslint-plugin-jsx-a11y\lib\rules\lang.js:47:58)
    at listeners.(anonymous function).forEach.listener (C:\Development\Projects\gatsby-starter-strict\node_modules\eslint\lib\util\safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (C:\Development\Projects\gatsby-starter-strict\node_modules\eslint\lib\util\safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (C:\Development\Projects\gatsby-starter-strict\node_modules\eslint\lib\util\node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (C:\Development\Projects\gatsby-starter-strict\node_modules\eslint\lib\util\node-event-generator.js:280:22)

@vhfmag
Copy link
Contributor Author

vhfmag commented Apr 20, 2019

Weird, this seems to be precisely the bug this patch was meant to fix. @kripod, mind to share the code that causes it?

@kripod
Copy link

kripod commented Apr 20, 2019

@vhfmag I linked my issue with code included in my previous comment.

@vhfmag
Copy link
Contributor Author

vhfmag commented Apr 22, 2019

The issue seems to be that LITERAL_TYPES in https://github.com/evcohen/jsx-ast-utils/blob/7bd0a47/src/values/expressions/index.js#L88-L126 does not include an extractor for TSNonNullExpression. I'm planning to work on it soon, but I'm not sure what is expected to be extracted. It's worth noting that the fix should also handle the TSAsExpression type.

Adding TSNonNullExpression: noop fixes the lint error, and adding TSNonNullExpression: value => value.expression.type doesn't throw but makes for a lint failure in https://github.com/kripod/gatsby-starter-strict.

@evcohen would you explain what is the expected behavior in this case? Thanks in advance!

@vhfmag
Copy link
Contributor Author

vhfmag commented Apr 22, 2019

@kripod #80

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Consider handling typescript-eslint-specific AST
5 participants