Allow JSX Import Source to be overwritten#47
Conversation
|
bump |
…actjs#48) * add named export to improve `"module": "nodenext"` compatibility * add proper types export * Update src/index.ts Co-authored-by: Jovi De Croock <decroockjovi@gmail.com> Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
marvinhagemeister
left a comment
There was a problem hiding this comment.
This PR has the right spirit, but does a whole lot more than just allowing jsxImportSource to be overwritten.
Is this referring to the additional 3 commits caused by fast forwarding the pull request to main? |
|
You need to rebase your changes. The diff is quite mangled. |
|
Should be fixed now. |
|
@Interpause the comment to leave prePublish in still stands 😅 we want this so when we use |
|
@JoviDeCroock FWIW, I would argue against Edit: I guess it facilitates installing from git, but meh. Do people do that often enough? |
package.json
Outdated
| "build": "rimraf dist && tsc && tsc -p tsconfig.cjs.json && node tools/postbuild.mjs", | ||
| "prepublishOnly": "npm run build" | ||
| "prepublishOnly": "npm run build", | ||
| "prepare": "npm run build" |
There was a problem hiding this comment.
This causes the build command to be run whenever a user installs our vite preset. Is this change necessary to allow JSX Import Source to be overwritten?
There was a problem hiding this comment.
According to the docs, I think it's only when the preset is installed through git? Which, of course, would require build be ran.
There was a problem hiding this comment.
I think it's not just that:
- Runs on local
npm installwithout any arguments
In either way this change doesn't seem necessary to allow JSX import source to be overwritten.
There was a problem hiding this comment.
I think it's not just that:
microbundle has a prepare script, for example, and it doesn't run on install (nor could it). "local" means this project, as in, if you were to clone this repo and run npm install.
Not the best wording from NPM, to be fair.
In either way this change doesn't seem necessary to allow JSX import source to be overwritten.
Yeah totally agree there, and I generally think it's a super annoying life cycle script anyways.
There was a problem hiding this comment.
I stand corrected! Looks like I made the wrong assumption.
There was a problem hiding this comment.
I am guessing to keep the pull request pure, you want to remove the changes to package.json?
There was a problem hiding this comment.
That, but also as it stands, you haven't provided an explanation for the change. Why are we adding a new script?
While we can guess about your intentions (as we've done above), for right now, it's a change without any rhyme nor reason.
There was a problem hiding this comment.
To be fair, the prepare script isn't needed if the package is installed from npm. However, as I needed to overwrite the import source for a project, and did not want to publish to npm, I added the prepare script so that installing via git would be possible.
|
Say, its been a while. Actually, I think the changes made to allow configuring |
|
Could you please consider to merge the PR, as the changes had been cleaned up |
| { | ||
| runtime: "automatic", | ||
| importSource: "preact", | ||
| importSource: jsxImportSource ?? "preact", |
There was a problem hiding this comment.
nullish coalescing while having no benefit over || operator in this case, still shouldn't break anyone's environment, as it's supported in Node.js 14 (oldest version that hasn't reached EOL)
This pull request solves #46 by adding
jsxImportSourcetoPreactPluginOptions. As a bonus, I changedprepublishOnlytoprepare, which allows the package to be installed directly via Git and should not affect its behaviour when being published to npm.