-
-
Notifications
You must be signed in to change notification settings - Fork 147
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(hmr): add in hmr capabilities #1400
Conversation
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.
Looks great! Can we have some demo gif pls π
I am against to add HMR inside conventions.
User can also write Aurelia 2 app without using our conventions in the toolchain at all. Aka, totally manual and verbose code. For the above two arguments, I suggest to invest the possibility to add HMR support in other place (or a standalone npm package like we did for Aurelia v1). |
23d93cc
to
8a0fb9c
Compare
I think plugin-conventions is just a naming problem and this code indeed belongs here. We shouldn't be making double processing a thing from our one plugin. There is an issue with a standalone package as it would be one file with not really any code (more an interpolated string). This is overkill. I would suggest just renaming conventions and letting it include more than just conventions in the plugin. @aurelia/cli |
I think not everyone would like to use the convention, though. I think it would be better to create multiple plugins (if not multiple packages) that focuses on a single aspect, and then an umbrella plugin to aggregate the config or whatever for the ease of use (although it can be postponed if super complicated or if the individual plugins can be easily used). IMO, it is important to ensure that HMR works individually without relying on convention. |
It doesn't rely on convention. HMR operates by itself entirely in the plugin-convention package. That is why I am thinking it is named improperly as it takes care of more than just one thing. We can seperate out into many packages, but there should be one entry into code manipulation I feel. |
I recall, quite some time ago, we talked about moving all package-cjs into individual repos. Those packages are not core packages. Putting them into monorepo actually hurts development more than helping, especially the cjs/esm mode build switching for local setup. We should revisit the idea before production release. |
With rollup, we no longer need to switch to build. Though we are still switching the When running test with mocha, we still need to do this because some reasons I'm not sure 100% yet. Running |
If you don't use "main": "dist/index.cjs",
"module": "dist/index.mjs",
"types": "dist/index.d.ts",
"exports": {
// works with require(...)
"require": "dist/index.cjs",
// works with import.
"import": "dist/index.mjs"
}, Ref: https://github.com/gorbjs/shared/blob/main/package.json |
@3cp thanks, I'll be applying that. |
@bigopon some more @aurelia/processor |
I've created a discussion for our rename here #1404 |
Codecov Report
@@ Coverage Diff @@
## master #1400 +/- ##
=======================================
Coverage 87.07% 87.07%
=======================================
Files 212 212
Lines 17505 17505
Branches 4106 4106
=======================================
Hits 15243 15243
Misses 2262 2262 π£ Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@brandonseydel BTW, I could not get my head around how to support vite prod build. Vite uses different path for dev and prod build. The prod build tries to parse our html as html, but we already transpiled html into js code. The whole rollup chain does not have concept of asset type, nor allowing changing extname in id (the file name). When you are on Vite, see if you can come up with any workaround, thx. For reference: the vite au2 plugin which works (kind of) in dev mode: |
Yea been fighting with vite some on the mangler side of things. It doesn't seem to want to listen when using esbuild..... |
@@ -27,7 +27,12 @@ export function preprocess( | |||
unit.filePair = path.basename(filePair); | |||
} | |||
} | |||
return preprocessHtmlTemplate(unit, allOptions); | |||
|
|||
const hasViewModel = Boolean(allOptions.jsExtensions.map(e => |
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.
@brandonseydel I might be confused, but I don't think this ever worked. We don't unit test hmr setup, so might missed this bug.
Boolean([false, false]) yields true
cc @bigopon
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.
@brandonseydel @bigopon my bad, I miss understood the code :(
Pull Request
π Description
π« Issues
π©βπ» Reviewer Notes
π Test Plan
β Next Steps