embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
330 stars 137 forks source link

[vite entry point] add gjs + gts extension for resolvable extensions #1679

Closed patricklx closed 5 months ago

patricklx commented 6 months ago

add gjs+gts as resolveable extensions for vite entry point asset file

ef4 commented 6 months ago

Hi, I need more detail to understand what you're trying to do here.

resolvableExtensions is trying to match existing ember-cli behaviors and those don't actually resolve gjs/gts. I suspect there might be better place to put this fix to achieve what you're trying to achieve.

patricklx commented 6 months ago

It's possible to have gts/gjs files for templates with ember-route-templates. It's possible to transform them before starting vite, but i would like vite to do the transform instead. Also because I'm working to make it work to load everything from app dir instead of rewritten-app. Which is mostly working now, except for this. So, it would only be required to pre-compile once to start dev (mostly)

ef4 commented 6 months ago

Sure but the gjs plugin you've also been working on already resolve gjs/gts. I still don't understand why it would be needed here in compat-app-builder too.

patricklx commented 6 months ago

Because of this: https://github.com/embroider-build/embroider/blob/d68fbec4931ad34c11d551d4e9cd7f81d97f370e/packages/compat/src/compat-app-builder.ts#L580 Which is responsible to build the entry point.. Which would miss route.gts for example

ef4 commented 6 months ago

That line is building vendor.js, which should not have support for gts files. I think you probably mean this one instead: https://github.com/embroider-build/embroider/blob/d68fbec4931ad34c11d551d4e9cd7f81d97f370e/packages/compat/src/compat-app-builder.ts#L758

which would influence the set of stuff that gets pushed into the app entrypoint. But yeah, I see what you mean.

I'm really not ready to merge this PR unless all of this:

Also because I'm working to make it work to load everything from app dir instead of rewritten-app.

is also something you're ready to land as stable, and I'm skeptical that that will be easy. We have been steadily working to eliminate a mandatory rewritten-app dir, but we know there are still a bunch of features that are depend on it. Without that, this feature doesn't actually do anything, since as you say the gts/gjs handling happens in the earlier stage.

patricklx commented 6 months ago

Yes, what I've been working on is that vite loads app files from app root directory But it still need the rewritten-app. I will try to finish it tomorrow. So it would be more like a intermediate solution. As you would need to rebuild when adding new routes or static files. But otherwise it would be able to instantly reload without needing rebuild through embroider

patricklx commented 6 months ago

i think this also makes sense without having vite load everything from root. it can load gts/gjs files directly and we can also then remove enableTypeScriptTransform in ember-cli-build as vite can do it. making hot reload even faster

ef4 commented 6 months ago

It's not OK to sometimes load from root and sometimes from the rewritten app, and so long as the files exist in the rewritten app they are resolvable.

If you have both, vite will duplicate them.

We are quite close to eliminating rewritten app entirely.

patricklx commented 6 months ago

Right, what i meant in my last statement is that embroider could just copy the gts/gjs files to the rewritten-app and have vite resolve them. But the issue would remain that the route.gts would be missing from the entry point defines. It would then lead to issues in the app, because ember would use the default router instead.

It's good to hear that rewritten-app is soon gone. Would that mean that vite will load from real app instead?

patricklx commented 6 months ago

Wip here: https://github.com/embroider-build/embroider/pull/1695

patricklx commented 5 months ago

reopen to separte out of big vite pr

patricklx commented 5 months ago

moved to #1718