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 JSX extension to @rollup/plugin-node-resolve options #524

Merged
merged 3 commits into from
Mar 11, 2020

Conversation

n3tr
Copy link
Contributor

@n3tr n3tr commented Feb 28, 2020

close #523

Add JSX extension to rollup @rollup/plugin-node-resolve plugin

The default extensions value doesn't include the .jsx and It causes an error when import jsx from js file.


Maintainer edit: happened to see it recently, so just adding a ref to developit/microbundle#571 which was created shortly after this in case that helps in future debugging or something

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR @n3tr ! Should go along nicely with all the other JSX support that's been added recently.

I haven't had a chance to test this out myself (just quickly looking it over), but makes sense.
A major point related to that is to add an automated test(s) for this. Should be able to put it in the build-default/syntax/ fixture directory

Also the title and commit should be renamed to "Add JSX extension ..." not just "Add extensions ..." which isn't really specific about the change. extensions is only needed because JSX is being added, the rest are defaults.

src/createRollupConfig.ts Show resolved Hide resolved
@n3tr n3tr changed the title Add extensions to @rollup/plugin-node-resolve options Add JSX extension to @rollup/plugin-node-resolve options Feb 29, 2020
@n3tr
Copy link
Contributor Author

n3tr commented Mar 11, 2020

@agilgur5 I added a comment and tests

@n3tr n3tr requested a review from agilgur5 March 11, 2020 03:48
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @n3tr . I wanted to make sure I had some time to play around with this before merging since we don't know what the root cause is (also I was just added as a collaborator yesterday, so I couldn't merge until then anyway).

I ran some tests and indeed with this set-up it only fails on the third import for some reason. When I changed JSX-import-JSX to actually be imported & exported itself as a variable, then suddenly it started failing to resolve as the first import. This is more robust of a test though, so let's leave this in.

I'm still not really sure why it fails on the third import or why this specific change is necessary (since like .ts and .tsx aren't resolved and Babel's DEFAULT_EXTENSIONS includes .jsx) but can merge this in as a fix until we figure out the root cause.

I tried changing some settings with rpts2, like adding allowJs: true and adding "**/*.js+(|x)" to the plugin include (different from tsconfig include) but those still failed so 🤷‍♂

@agilgur5 agilgur5 merged commit 735f301 into jaredpalmer:master Mar 11, 2020
@agilgur5
Copy link
Collaborator

@allcontributors please add @n3tr for code, test, bugs

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @n3tr! 🎉

paul-vd pushed a commit to EezyQuote/tsdx that referenced this pull request Dec 1, 2020
…#524)

* Add JSX extension to @rollup/plugin-node-resolve options

* Add test for JSX chaining import

* Correct comment link
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.

Could not resolve .jsx file when import from other .jsx
2 participants