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

Fix: transpiling React.createElement into Preact h behavior #7435

Merged

Conversation

Shinyaigeek
Copy link
Contributor

↪️ Pull Request

Hi team 👋

Fixes: #7433

jsxImportSource will be determined with reactLib variable, but reactLib variable is always “react” string literal if alias.react is setted. I think jsx-runtime (and automatic transformation) is not related.

I fixed this

💻 Examples

package.json

{
  "alias": {
    "react": "preact/compat"
  }
}

🚨 Test instructions

I made a test case to check jsx runtime transformation with preact with alias.react package.json.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@height
Copy link

height bot commented Dec 11, 2021

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@Shinyaigeek Shinyaigeek force-pushed the fix/correct-jsx-runtime-import-source branch from dc89141 to ca600b4 Compare December 11, 2021 19:43
@@ -126,6 +126,37 @@ const SCRIPT_ERRORS = {
},
};

function convertAliasReactIntoPragma(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to convert preact/compat into preact pragma, I made a function to do it, but I feel this approach is a little muddy. If you have another idea to implement this, I will be happy if you comment on it.

@Shinyaigeek Shinyaigeek changed the title Fix/correct jsx runtime import source Fix: transpiling React.createElement into Preact h behavior Dec 11, 2021
@Shinyaigeek
Copy link
Contributor Author

Shinyaigeek commented Dec 13, 2021

I think this approach is wrong. I thought #7433 behavior is caused by this line(https://github.com/parcel-bundler/parcel/blob/v2/packages/transformers/js/src/JSTransformer.js#L160) in which reactLib is fixed to react when aliased . However, As this comment (#7435 (comment)), reactLib should be fixed to react when aliased.

I think the cause is not this line, but these line(https://github.com/parcel-bundler/parcel/blob/v2/packages/transformers/js/src/JSTransformer.js#L201). In these line, JSTransformer decides whether parcel (and backend swc) should automatically transform createElement to new JSX Runtime API, and decides which JSXImportSource should be used.
The cause for this issue is that reactLib is fixed to react so this processing runs in the case only react. I feel I should fix these line to insert the specific processing when pkg.alias is configured.

It's 3am in my timezone, so once I back this PR to draft and sleep, and I’ll work tomorrow 🙇.

@Shinyaigeek Shinyaigeek marked this pull request as draft December 13, 2021 18:20
@Shinyaigeek Shinyaigeek marked this pull request as ready for review December 14, 2021 15:23
@Shinyaigeek
Copy link
Contributor Author

I fixed 👍

@mischnic
Copy link
Member

Does the current version still have the problem that reactLib (and therefore jsxImportSource) is set to "preact" even though it should be "react"?

I think something like this should work. The other react-like libraries don't support this automatic runtime anyway

--- packages/transformers/js/src/JSTransformer.js
+++ packages/transformers/js/src/JSTransformer.js
@@ -199,11 +199,15 @@ export default (new Transformer({
         jsxImportSource = compilerOptions?.jsxImportSource;
         automaticJSXRuntime = true;
       } else if (reactLib) {
-        let automaticVersion = JSX_PRAGMA[reactLib]?.automatic;
+        let effectiveReactLib =
+          (pkg?.alias && pkg.alias['react']) === 'preact/compat'
+            ? 'preact'
+            : reactLib;
+        let automaticVersion = JSX_PRAGMA[effectiveReactLib]?.automatic;
         let reactLibVersion =
-          pkg?.dependencies?.[reactLib] ||
-          pkg?.devDependencies?.[reactLib] ||
-          pkg?.peerDependencies?.[reactLib];
+          pkg?.dependencies?.[effectiveReactLib] ||
+          pkg?.devDependencies?.[effectiveReactLib] ||
+          pkg?.peerDependencies?.[effectiveReactLib];
         let minReactLibVersion =
           reactLibVersion != null && reactLibVersion !== '*'
             ? semver.minVersion(reactLibVersion)?.toString()

@Shinyaigeek
Copy link
Contributor Author

Shinyaigeek commented Dec 16, 2021

As you say, it's not JSTransformation's responsibility to resolve the alias, I overlooked it. There are many libraries that support jsxRuntime, but only "preact" supports {reactLib}/jsx-runtime alias, and I feel that even if more library support that alias and parcel supports it in the future, It's enough to abstract it to at that time. I fixed it in e05804a 👍

Thank you for helping!

Comment on lines 194 to 195
let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('_jsxRuntime.jsx("div"'));
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also add this to make sure it's actually importing from React (using a Regex because react/jsx-runtime is a substring of preact/jsx-runtime).

Suggested change
let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(file.includes('_jsxRuntime.jsx("div"'));
let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
assert(/\Wreact\/jsx-runtime\W/.test(file));
assert(file.includes('_jsxRuntime.jsx("div"'));

Copy link
Contributor Author

@Shinyaigeek Shinyaigeek Dec 18, 2021

Choose a reason for hiding this comment

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

Sure. I fixed in 4efec78 👍 Thanks!

@devongovett devongovett merged commit 931ecd5 into parcel-bundler:v2 Dec 19, 2021
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.

Automatic JSX runtime doesn't work when React is aliased to Preact
3 participants