Skip to content

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

Merged
agilgur5 merged 3 commits into
jaredpalmer:masterfrom
n3tr:node-resolve-ext
Mar 11, 2020
Merged

Add JSX extension to @rollup/plugin-node-resolve options#524
agilgur5 merged 3 commits into
jaredpalmer:masterfrom
n3tr:node-resolve-ext

Conversation

@n3tr

@n3tr n3tr commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

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

@agilgur5 agilgur5 left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/createRollupConfig.ts
@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

n3tr commented Mar 11, 2020

Copy link
Copy Markdown
Contributor Author

@agilgur5 I added a comment and tests

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

@agilgur5 agilgur5 left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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

@allcontributors

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