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

Support .tsx and .jsx files from CLI #448

Merged
merged 6 commits into from
Aug 26, 2019

Conversation

ricardobeat
Copy link
Contributor

Despite the transforms being supported, the CLI will currently only compile .ts files. This makes sure the correct file extensions are used when running something like sucrase ./src -d ./out --transforms imports,typescript,jsx.

Despite the transforms being supported, the CLI will currently only compile `.ts` files. This makes sure the correct file extensions are used when running something like `sucrase ./src -d ./out --transforms imports,typescript,jsx`.
Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

So sorry for the delay! #458 was just submitted, which is very similar to this one, so I'll accept this or the other one. Looks like this one has an undefined variable, crash though. (Would be nice if the CLI had tests, though I guess it did cause a CI failure because it's implicitly tested by the build system.)

src/cli.ts Outdated Show resolved Hide resolved
@ricardobeat
Copy link
Contributor Author

Fixed - the other PR is almost exactly the same, only uses a regexp instead of path to handle the extension. Apologies for the mistake - for some reason yarn build only works in the master branch, though tests are now passing.

@alangpierce
Copy link
Owner

Thanks!

Looks like it was still having issues because it wasn't the full path, just the filename portion. I switched to the regex approach in the other PR, unclear which one is "best", but certainly the regex is a bit more concise.

The array approach seems a little more extensible for future things like .mjs,
for example.
Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

@ricardobeat sorry for the delay! I made one tweak and will merge when the build passes.

@codecov-io
Copy link

Codecov Report

Merging #448 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #448   +/-   ##
=======================================
  Coverage   81.42%   81.42%           
=======================================
  Files          50       50           
  Lines        5180     5180           
  Branches     1208     1207    -1     
=======================================
  Hits         4218     4218           
  Misses        669      669           
  Partials      293      293
Impacted Files Coverage Δ
src/cli.ts 0% <0%> (ø) ⬆️

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 5ae23c8...b3fccb7. Read the comment docs.

@alangpierce alangpierce merged commit 6ed6dff into alangpierce:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants