-
Notifications
You must be signed in to change notification settings - Fork 2
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: use react-jsx transform and support ES2021 types #12
Conversation
f3e803d
to
fb92e61
Compare
tsconfig.json
Outdated
"jsx": "react", | ||
"lib": ["dom", "es6", "dom.iterable"], | ||
"jsx": "react-jsx", | ||
"lib": ["DOM", "ES6", "DOM.Iterable", "ES2021"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, here's where I've ended up in frontend-base:
"jsx": "react-jsx",
"lib": [
"dom",
"dom.iterable",
"esnext"
],
Admittedly I don't 100% comprehend exactly including things here gets you. Do you really need ES6 and ES2021?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly I don't 100% comprehend exactly including things here gets you. Do you really need ES6 and ES2021?
On a quick test locally, it does appear that specifying a later spec, you don't need the older ones. E.g., ESNext
also resolves the .replaceAll()
type issue I'm seeing, without explicitly needing to specify ES2021
.
So yeah, probably don't need "ES6" and "ES2021". Any objection if I change to "esnext" to match what you have in frontend-base? I don't think it has too much downside? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related, I also wonder if we even need "DOM.Iterable" if we're including all of "DOM" already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I'm reading, adding stuff to lib just changes what type definitions are available in your source code, so a more permissive list here is kind of okay. The 'target' determines what features get transpiled down ('down-leveled' in the TS documentation). So since our target is ES6, we should be good.
I wasn't quite clear on DOM/DOM.Iterable. I do know that create-react-app generates a tsconfig.json with both. :) (In fact, it matches what I put in frontend-base, which is why I did it...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, this is a create-react-app
tsconfig.json file:
{
"compilerOptions": {
"target": "es5",
"lib": [
"dom",
"dom.iterable",
"esnext"
],
"allowJs": true,
"skipLibCheck": true,
"esModuleInterop": true,
"allowSyntheticDefaultImports": true,
"strict": true,
"forceConsistentCasingInFileNames": true,
"noFallthroughCasesInSwitch": true,
"module": "esnext",
"moduleResolution": "node",
"resolveJsonModule": true,
"isolatedModules": true,
"noEmit": true,
"jsx": "react-jsx"
},
"include": [
"src"
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[inform] I've updated to match the lib
as frontend-base / CRA configures it (leaving in the DOM
and DOM.iterable
).
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Part 1.
Problem
I introduced a
.tsx
file that renders JSX in an MFE that uses the latest@openedx/frontend-build
. This file does not include animport React from "react";
, given the MFE runs React 17 where the explicitReact
import is no longer required (article).Without including the explicit
React
import, the following error appears:However, given in React 17 the
React
import is not required in.jsx
files,.tsx
files ideally also should be able to withhold theReact
import, too.Discovery
The
jsx
option intsconfig.json
currently relies on a legacy runtimereact
, which emits JS usingReact.createElement
. Since React 17, a new JSX transform was introduced that allows using React without explicitly importingReact
.However, for usage with TypeScript, the
tsconfig.json
file needs to opt into using the new transform via thejsx
option (documentation).Options:
react-jsx
: Emit.js
files with the JSX changed to_jsx
calls optimized for productionreact-jsxdev
: Emit.js
files with the JSX changed to_jsx
calls for development onlypreserve
: Emit.jsx
files with the JSX unchangedreact-native
: Emit.js
files with the JSX unchangedreact
: Emit.js
files with JSX changed to the equivalentReact.createElement
callsFor example, this sample code gets transformed as follows:
Legacy React runtime: "react"
React with new JSX transform: "react-jsx"
Solution
Extending this base config's
jsx
option in a consuming MFE'stsconfig.json
to use the newreact-jsx
resolves the error around theReact
global.Part 2
The other change in this PR is to add support for "ES2021" types, to enable support for string functions like
.replaceAll()
in TypeScript files by extending thelib
option intsconfig.json
.Doing so in a consuming MFE's
tsconfig.json
resolves the TypeScript error about.replaceAll()
, but this feels like a fairly common syntax consuming repos may use so makes sense to update the base config directly.