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

feat(sucrase): resolve .tsx files #448

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Conversation

flipsasser
Copy link
Contributor

Rollup Plugin Name: sucrase

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

Description

The @rollup/plugin-sucrase plugin does not resolve .tsx files. This PR patches that behavior to allow the use of @rollup/plugin-sucrase in React / TypeScript projects.

It includes a test that imports a TSX file and snapshots the JSX output. It works. Which is great!

Here's a different way of saying it:

  • Expected behavior: using @rollup/plugin-sucrase with transforms: ["typescript", "jsx"] will correctly import both .ts and .tsx files.
  • Actual behavior: @rollup/plugin-sucrase cannot resolve .tsx files; only .ts.

Sorry, something went wrong.

const resolvedFilename = [`${resolved}.ts`, `${resolved}/index.ts`].find((filename) =>
fs.existsSync(filename)
);
const resolvedFilename = [
Copy link
Member

Choose a reason for hiding this comment

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

if for .ts both ${resolved}.ts and ${resolved}/index.ts are checked then I believe that similar should be checked for .tsx, so both ${resolved}.tsx and ${resolved}/index.tsx

Copy link
Collaborator

Choose a reason for hiding this comment

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

@flipsasser please take a look at this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey sorry somehow I missed this - will fix. Thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It now follows the same pattern as ts imports

Flip Sasser added 2 commits June 23, 2020 14:46

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@shellscape shellscape merged commit e6f3f06 into rollup:master Jun 24, 2020
@flipsasser
Copy link
Contributor Author

@Andarist what is the NPM release schedule like, and can I assist in getting this update published? Please let me know if I can help. Thanks!

LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* Resolve .tsx files

* Load from index-level tsx files (just like TS)

Co-authored-by: Flip Sasser <flip@inthebackforty.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants