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

refactor: first class HMR support #795

Merged
merged 15 commits into from
Feb 16, 2024
Merged

refactor: first class HMR support #795

merged 15 commits into from
Feb 16, 2024

Conversation

ineshbose
Copy link
Collaborator

@ineshbose ineshbose commented Jan 20, 2024

resolves #682

Planned for 6.12.0. This PR will be open for a while.

Test on Stackblitz: https://stackblitz.com/github/nuxt-modules/tailwindcss/tree/first-class-hmr

Still very much in progress, but any efforts on testing this would be appreciated;
there are a few issues like importing CJS/ESM/TS in one file, template reloading, merging strategy, etc..

src/module.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
src/templates.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/templates.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
playground/nuxt.config.ts Outdated Show resolved Hide resolved
@ineshbose
Copy link
Collaborator Author

ALSO NEED TO TEST WITH WEBPACK

@ineshbose
Copy link
Collaborator Author

ineshbose commented Jan 20, 2024

It's also possible that this has broken the normal HMR for other files 🥲
likely due to layers/loading CJS in config

src/config.ts Outdated Show resolved Hide resolved
src/templates.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
@ineshbose
Copy link
Collaborator Author

ineshbose commented Jan 21, 2024

Another thing to note: plugins will also work with Tailwind and intellisense (they can't be added through hooks as they won't be serialized in the function) - this could mean a few different things.

One of the tests for this is failing where we have require('something-that-doesnt-exist') in the plugins (and that is reasonable IMO)

src/config.ts Outdated Show resolved Hide resolved
src/runtime/merger.mjs Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
@ineshbose
Copy link
Collaborator Author

CHANGES NEEDED IN
NUXT/UI: pass config as template and in configPath
NUXT/UI-PRO: pushing to content

@ineshbose
Copy link
Collaborator Author

CHANGES NEEDED IN
NUXT/UI: pass config as template and in configPath
NUXT/UI-PRO: pushing to content

the update to nuxt/ui exposes a bug in resolveModulePaths as that respects nuxt.options._layers over installModule options. Plus, I see it ignore layers' inline configs too (maybe intentional)

@ineshbose
Copy link
Collaborator Author

CHANGES NEEDED IN NUXT/UI: pass config as template and in configPath NUXT/UI-PRO: pushing to content

refer: nuxt/ui#1272, https://github.com/nuxt/ui-pro/pull/205

the update to nuxt/ui exposes a bug in resolveModulePaths as that respects nuxt.options._layers over installModule options. Plus, I see it ignore layers' inline configs too (maybe intentional)

refer: c7fe392

@ineshbose
Copy link
Collaborator Author

I think I should change my approach and not use defu to merge updates with the fresh config but rather generate more code in the template.

@ineshbose
Copy link
Collaborator Author

Wow. That worked surprisingly well - didn't even need to use knitwork.

@ineshbose ineshbose marked this pull request as ready for review February 14, 2024 11:22
src/config.ts Outdated Show resolved Hide resolved
Copy link

nuxt-studio bot commented Feb 14, 2024

Live Preview ready!

Name Edit Preview Latest Commit
TailwindCSS Edit on Studio ↗︎ View Live Preview 2c0d664

@ineshbose
Copy link
Collaborator Author

seems like https://github.com/unjs/c12?tab=readme-ov-file#watching-configuration was also an option... or maybe it wouldn't have worked the same way as we need to generate code after hook manipulation anyway. Will evaluate after working on #808

@ineshbose ineshbose merged commit 25b9f9e into main Feb 16, 2024
2 of 5 checks passed
@ineshbose
Copy link
Collaborator Author

@benjamincanac would you be able to try this out with nuxt/ui using nightly?

@TechAkayy
Copy link

TechAkayy commented Feb 22, 2024

@benjamincanac would you be able to try this out with nuxt/ui using nightly?

Hi @benjamincanac,

I have tested this merge in my nuxt-ui sample app and tagged you here - #682 (comment).

But, the repo that I used in my testing is not using the latest nuxt-ui, and doesn't use nuxt-ui-pro, so would be great if you can try this out on your end, thanks in advance.

Copy link
Member

@ineshbose Just tried it out and encountered the same errors as @TechAkayy: Pre-transform error: [postcss] Cannot read properties of undefined (reading 'includes').
CleanShot 2024-02-22 at 10.57.40@2x.png

Is there some breaking changes to take into account? I've made this draft PR if you want to try yourself: nuxt/ui#1397

@ineshbose
Copy link
Collaborator Author

Thanks for reporting @benjamincanac; I've pushed workaround/fix on the same PR. Lets continue discussion in #682.

ineshbose added a commit that referenced this pull request Feb 28, 2024
ineshbose added a commit that referenced this pull request Feb 28, 2024
@ineshbose ineshbose mentioned this pull request Feb 28, 2024
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.

HMR for config imports and latency improvement
3 participants