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(remix-dev): add jsxImportSource config option #2216

Closed
wants to merge 11 commits into from
Closed

feat(remix-dev): add jsxImportSource config option #2216

wants to merge 11 commits into from

Conversation

jolle
Copy link

@jolle jolle commented Mar 5, 2022

Libraries such as emotion (via @emotion/react) are commonly used with the tsconfig option jsxImportSource, which changes the default React.createElement calls to the specified source automatically. Because esbuild does not support this out-of-the-box (see evanw/esbuild#718), by using a dynamically created shim file it is possible to emulate this behavior. Although an inline pragma works, it is required to add this to every file instead of being automatically detected.

As esbuild does not support inline code injection (see evanw/esbuild#1169), this PR creates a temporary directory with a temporary file for the custom shim. While this is not very optimal, it is the best option I've found (didn't seem like esbuild plugins could do this?). In this PR, the tsconfig.json file is read (if one exists), and a shim is created based on jsxImportSource, if one is specified. A new config option, customJSXShimPath, is exposed as well if a custom path to the JSX factory is required for the shim.

This pull request also adds tests for this new behavior, and refactors the tests of build-test.ts that were being skipped (because they didn't work). Due to ESM-related issues and conflicts with the Jest environment, a new, simpler app needed to be created without any ESM-only modules, and some modules needed to be mocked during build-time. The JSX factory tests run the built output within the test in a "sandbox" where the calls to the JSX factory can be monitored.

This is a quite big new feature – apologies for not creating a discussion first!

Closes: #1633. Related to: #2209.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 5, 2022

Hi @jolle,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 5, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@machour
Copy link
Collaborator

machour commented Mar 20, 2022

@jolle Can you please update this PR to fix conflicts? 🙏🏼

@machour machour added the needs-response We need a response from the original author about this issue/PR label Mar 20, 2022
@jolle
Copy link
Author

jolle commented Mar 20, 2022

Merge conflicts have been fixed. I noticed there has been changes to the testing env that causes the new tests to fail (related to fixtures trying to import the remix package). I'll try to fix the tests soon, though the solution isn't immediately obvious to me – I'll have to go through the changes and find what has broken the fixtures!

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Mar 20, 2022
@MichaelDeBoey MichaelDeBoey changed the title feat: add custom JSX factory support feat(remix-dev): add custom JSX factory support Mar 28, 2022
@MichaelDeBoey MichaelDeBoey added package:dev needs-response We need a response from the original author about this issue/PR labels Mar 28, 2022
@MichaelDeBoey MichaelDeBoey removed the needs-response We need a response from the original author about this issue/PR label Jun 4, 2022
@MichaelDeBoey
Copy link
Member

@jolle Please rebase your branch onto latest dev & fix conflicts + tests.
That way the team has a clean plate to start reviewing if they want to have this merged into the library or not, thanks! 🙏

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jun 4, 2022
@MichaelDeBoey MichaelDeBoey changed the title feat(remix-dev): add custom JSX factory support feat(remix-dev): add customJSXShimPath config option Jun 4, 2022
Comment on lines +10 to +28
// Mock ESM-only modules as these are not well-supported in
// the Jest environment available.
jest.mock("xdm", () => ({
__esModule: true,
compile: () => ({}),
}));
jest.mock("remark-frontmatter", () => ({
__esModule: true,
}));
jest.mock("tsconfig-paths", () => ({
__esModule: true,
default: {
loadConfig() {
return {
resultType: "failed",
};
},
},
}));
Copy link
Author

Choose a reason for hiding this comment

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

This is not optimal but regardless of what I did to Babel or Jest these modules seemed to break Jest one way or another.

@jolle
Copy link
Author

jolle commented Jun 4, 2022

I've resolved the merge conflicts and fixed the tests. In addition to the caveat above, I downgraded strip-json-comments as v4 was released only to make it an ESM (which is breaking the Jest env; v3 does not have this issue).

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Jun 4, 2022
@jolle jolle changed the title feat(remix-dev): add customJSXShimPath config option feat(remix-dev): add and support TS jsxImportSource config option Jun 7, 2022
@jolle
Copy link
Author

jolle commented Jun 7, 2022

I have removed the temporary file system injection for adding the custom JSX factory; instead, the import source is loaded by adding a @jsx pragma header to every file (which is supported by esbuild) using a plugin.

@MichaelDeBoey MichaelDeBoey changed the title feat(remix-dev): add and support TS jsxImportSource config option feat(remix-dev): add jsxImportSource config option Jun 7, 2022
@luishdz1010
Copy link

This now works natively on esbuild, adding:

"overrides": {
    "@remix-run/dev": {
      "esbuild": "0.15.2"
    }
  }

To package.json (and using npm) works without any configs.

@jolle
Copy link
Author

jolle commented Dec 30, 2022

Support for automatic jsxImportSource detection in esbuild was added in v0.14.51 (evanw/esbuild#2349), so this is no longer necessary.

@jolle jolle closed this Dec 30, 2022
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

4 participants