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

Open
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@vhfmag
Copy link

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

vhfmag added some commits Feb 6, 2019

@ljharb

ljharb approved these changes Feb 6, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 6, 2019

Coverage Status

Coverage increased (+0.02%) to 98.844% when pulling 7e2cca5 on vhfmag:support-typescript-non-null-expression into e4c0312 on evcohen:master.

@vhfmag

This comment has been minimized.

Copy link
Author

vhfmag commented Feb 6, 2019

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

@ljharb

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

vhfmag added some commits Feb 6, 2019

Revert "Remove babylon"
This reverts commit 9f1a9c8.
@ljharb

This comment has been minimized.

Copy link
Collaborator

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 added some commits Feb 7, 2019

@vhfmag

This comment has been minimized.

Copy link
Author

vhfmag commented Feb 7, 2019

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

@ljharb

ljharb approved these changes Feb 7, 2019

@ljharb ljharb requested a review from evcohen Feb 7, 2019

Show resolved Hide resolved __tests__/helper.js Outdated
Improve eslint usage
Co-Authored-By: vhfmag <victorhfmag@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment