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.

Sign up for GitHub

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: Strips jsxFactory from tsconfig if --importSource is set #988

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: Strips jsxFactory from tsconfig if --importSource is set #988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76fba2c
fix: Conditionally sets jsxFactory & jsxImportSource in tsconfigs
rschristian Aug 10, 2022
e4d8db3
docs: Adding changeset
rschristian Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/thick-radios-jump.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'microbundle': patch
---

Fixes tsconfig merge issue with jsxImportSource and default jsxFactory value
15 changes: 12 additions & 3 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export default async function microbundle(inputOptions) {
options.pkg.name = pkgName;

if (options.sourcemap === 'inline') {
// eslint-disable-next-line no-console
console.log(
'Warning: inline sourcemaps should only be used for debugging purposes.',
);
Expand Down Expand Up @@ -549,9 +550,16 @@ function createConfig(options, entry, format, writeMeta) {
...(options.generateTypes !== false && {
declarationDir: getDeclarationDir({ options, pkg }),
}),
jsx: 'preserve',
jsxFactory: options.jsx,
jsxFragmentFactory: options.jsxFragment,
...(options.jsxImportSource
? {
jsx: 'react-jsx',
jsxImportSource: options.jsxImportSource,
}
: {
jsx: 'preserve',
Copy link
Owner

Choose a reason for hiding this comment

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

should this be preserve, or react?

Copy link
Collaborator Author

@rschristian rschristian Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

preserve should be correct, though from my quick tests I don't think it makes a difference either way.

preserve is to be used when having another tool transpile JSX (e.g., babel), which is what we do. react on the other hand has TSC transpile it itself. TS Docs

I think, at best, react would do nothing, and at worst, it'd slow down type generation due to trying to handle JSX which we don't need. No one's raised any issues regarding preserve so I'd assume it's fine.

Edit: Depends on what we want, I suppose. I'd prefer to have TS handle as little as possible and preserve is already the default behavior. I didn't originally realize we did have TS handle JSX.

jsxFactory: options.jsx,
jsxFragmentFactory: options.jsxFragment,
}),
Comment on lines +553 to +562
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Theoretically we would be fine to leave jsx untouched & remove that first piece of the ternary, however, that relies on the assumption that the user does have a tsconfig.json and that it's correctly set.

Conditionally adding jsxFactory (and jsxFragmentFactory) is enough to quell the errors we were getting in the linked issue, but when using --jsxImportSource with TS, the tsconfig.json will need to have different values. While users in all likelihood will have a tsconfig.json that's correctly configured in that case, if they do not, their typings will contain any where JSX references should be.

Setting these configurations ourselves, at least as defaults, allows us to make fewer assumptions about users' configs that are external to Microbundle (and hopefully will raise fewer issues).

},
files: options.entries,
},
Expand Down Expand Up @@ -642,6 +650,7 @@ function createConfig(options, entry, format, writeMeta) {
options.visualize && visualizer(),
// NOTE: OMT only works with amd and esm
// Source: https://github.com/surma/rollup-plugin-off-main-thread#config
// eslint-disable-next-line new-cap
useWorkerLoader && (format === 'es' || modern) && OMT(),
/** @type {import('rollup').Plugin} */
({
Expand Down