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

vite dev/test/build from app #1695

Closed
wants to merge 55 commits into from

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Dec 1, 2023

this can now already load most from root folder ./app and tests folder instead of rewritten_app folder. a few hacks where required.

the only files read from rewritten_app are :

  • config/environment.js
  • assets/test-support.js
  • assets/*.css
  • index.html

most problematic where asset loading.
vite does not allow loading static files from outside of root dir, so setting root to /app will not work.
using the CWD as root, will not load the index.html file.

this is where the custom middleware comes in to handle this parts.

  • load index.html foim app dir
  • load assets from public dir
  • load assets from node-module package
  • load from rewritten-packages

missing parts to allow editing live reload:

  • environment.js -> i think we should just call the vite-app/config/environment.js again at runtime (browser) and merge it again , like this, developers can edit the env and have it load by vite.
  • assets/vite-app.js
  • transform index.html in vite --> need replacements for content-for in a separate file so we can do it our selfs and expose api to build index.html
  • working tests
  • working vite build
  • rollup plugin for assets
  • auto rebuild when lock file changed or no . embroider dir

@patricklx patricklx marked this pull request as ready for review December 4, 2023 11:30
@patricklx patricklx changed the title vite load from app vite dev/test/build from app Dec 5, 2023
@patricklx
Copy link
Contributor Author

patricklx commented Dec 5, 2023

some tests are failing for unknown reason.
when I run test with pnpm test "--" "--filter" "release-compat-namespaced-app" it fails
but when I run it with node ./node_modules/qunit/bin/qunit.js --require ts-node/register *-test.ts "--filter" "release-compat-namespaced-app" it works...

@patricklx
Copy link
Contributor Author

Should i add a Test for vite app?
Can just be vite build

@patricklx
Copy link
Contributor Author

patricklx commented Dec 5, 2023

@ef4 this is ready for review

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

Thanks for giving this a go. Your spike showing this working is a helpful guide for several things, especially the middleware setup.

But this is not mergeable as is. A lot of stuff here is hard-coded in ways that might work in your app but definitely break in others. And @embroider/vite should only need to do vite-specific things.

Can we find time for you to pair with either me or @mansona? Because your help on this would be great, but it's so far not aligned with what we're trying to do. The effort that went into this PR could go directly into fixing these things in @embroider/core instead of hacking around them.

@@ -52,6 +52,7 @@
"@ember/test-helpers": "^2.9.1",
"@embroider/compat": "workspace:*",
"@embroider/core": "workspace:*",
"@embroider/shared-internals": "workspace:*",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have some unrelated changes in here. I don't see anything in this PR that would need this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, not sure why tests where suddenly failing. It was failing we this missing dependency...

@@ -91,7 +92,8 @@
"qunit": "^2.19.1",
"qunit-dom": "^2.0.0",
"typescript": "^5.1.6",
"webpack": "^5.74.0"
"webpack": "^5.74.0",
"semver": "^7.5.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, failing tests because of this missing dependency... Not sure why.

return new Promise((resolve, reject) => {
const child = fork('./node_modules/ember-cli/bin/ember', ['build']);
child.on('exit', code => (code === 0 ? resolve() : reject()));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the best place to manage rebuilds of the addons. This kind of rebuild is extremely expensive compared with ember s.

We could still do the persistent caching you have here in order to avoid even a first-time build of the addons if none have changed since your last build. But that logic would go some place more like

export function convertLegacyAddons(compatApp: CompatApp) {

Copy link
Contributor Author

@patricklx patricklx Dec 5, 2023

Choose a reason for hiding this comment

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

Yes, well it would also be quite rare to do build. Only if .embroider folder does not exist and if package-lock changed and index.html change.
Though i would like to rebuild the index.html only instead of full build.


export function build(): Plugin {
let resolverLoader = new ResolverLoader(process.cwd());
const engine = resolverLoader.resolver.options.engines[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

engines other than the first need this feature too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

packages/vite/src/build.ts Outdated Show resolved Hide resolved
server.watcher.add('./' + f);
}
});
server.watcher.add('./app/index.html');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't actually think we should bother supporting app/index.html. It's unnecessarily weird when we could just follow the normal vite convention and put index.html at the root.

It's not like people can use their original app/index.html anyway. Its format must change to work with this, if I'm reading correctly.

Copy link
Contributor Author

@patricklx patricklx Dec 5, 2023

Choose a reason for hiding this comment

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

Sounds good. Currently I'm rewrite it in rewriteHtml and just load the one kn rewritten-app.
I move tge index.html to the root then

Copy link
Contributor Author

@patricklx patricklx Dec 6, 2023

Choose a reason for hiding this comment

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

moving to root will not work, because the ember build will not work... unless we just put an empty index.html there... but then we will have 2 index.html files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think i found an option to ember-cli to set index.html path. Will try that

@@ -0,0 +1,125 @@
import { relative } from 'path';
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this entry generation is adding a second implementation for us to maintain instead of fixing the implementation we have. It doesn't belong in packages/vite because it's not vite-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I can look to replace the code.
webpack should support the same glob pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The glob pattern is only supported by vite...
Although there us a custom loader for webpack, it has a different syntax.
I think we should keep this for the app entries.
The addon entry file can be reverted to use the generated one by ember build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replacing this with compat app builder

packages/vite/src/virtual-files.ts Outdated Show resolved Hide resolved
packages/vite/src/virtual-files.ts Outdated Show resolved Hide resolved
packages/vite/src/virtual-files.ts Outdated Show resolved Hide resolved
@patricklx
Copy link
Contributor Author

I'm working now on using compat app builder to rebuild html and app assets when relevant files are added/removed. Looks working fine.

@patricklx patricklx mentioned this pull request Dec 7, 2023
5 tasks
@patricklx
Copy link
Contributor Author

patricklx commented Dec 7, 2023

@ef4 I made some changes

  • index.html at root
  • Asset related stuff is using compat app builder , i think like this we have a good base to possibly restructure internal code
  • added some more json files with info to use thr compat app builder to rebuild assets when relevant files changed
  • there where some issues when switching between app & tests. I fixed it by invalidating all.

@@ -57,6 +57,7 @@ import type { TransformOptions } from '@babel/core';
import { MacrosConfig } from '@embroider/macros/src/node';
import SourceMapConcat from 'fast-sourcemap-concat';
import escapeRegExp from 'escape-string-regexp';
import { configReplacePatterns } from 'ember-cli/lib/utilities/ember-app-utils';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im not sure if there is another place where embroider does the replacements

@patricklx
Copy link
Contributor Author

looks like the pr #1686 broke this...
Cannot find components/example. Any idea why?

@ef4
Copy link
Contributor

ef4 commented Dec 9, 2023

Perhaps it was relying on the resolvableExtensions in the node resolver, which is not there anymore after #1686.

For normal module imports that would not be an issue, since we can just configure vite to resolve .ts. But it is potentially an issue for discovering components in non-strict templates, since we're using nodeResolve there.

That gives me an idea: we could probably use defaultResolve instead of nodeResolve for component discovery. That would mean that our component search would understand .ts automatically when the prevailing packager does.

@patricklx
Copy link
Contributor Author

Also, I needed to add back the addons.ts for the depOptimizer as otherwise i was getting duplicates for glimmer-vm which does allow it.

@patricklx
Copy link
Contributor Author

@mansona maybe you can already have a look at this now.
I tried to implement the required changes without affecting normal builds

@@ -200,7 +209,7 @@ export default class CompatApp {

private get indexTree() {
let indexFilePath = this.legacyEmberAppInstance.options.outputPaths.app.html;
let index = buildFunnel(this.legacyEmberAppInstance.trees.app, {
let index = buildFunnel(this.legacyEmberAppInstance.trees.indexHtml || this.legacyEmberAppInstance.trees.app, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

required to have index.html at root for vite

@@ -83,7 +83,7 @@ export class PreparedEmberHTML {
implicitTestStyles: Placeholder;

constructor(private asset: EmberAsset) {
this.dom = new JSDOM(readFileSync(asset.sourcePath, 'utf8'));
this.dom = new JSDOM(asset.source || readFileSync(asset.sourcePath, 'utf8'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

used to load in-memory html content

@@ -532,22 +532,41 @@ export class Resolver {
}

private *componentTemplateCandidates(inPackageName: string) {
yield { prefix: '/templates/components/', suffix: '' };
yield { prefix: '/components/', suffix: '/template' };
yield { prefix: '/templates/components/', suffix: '.hbs' };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to add extension and /app prefix since component resolution is only done with nodeResolver, can revert when it uses defaultResolve

@@ -1041,10 +1060,10 @@ export class Resolver {
// but then come back to the original location here in the fallback when the
// rehomed request fails
let movedPkg = this.packageCache.maybeMoved(pkg);
if (movedPkg !== pkg) {
if (movedPkg !== pkg && !pkg.isV2App()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if its a app then no need to error here. (in vite, app is at root, and everything is loaded from root)

@@ -0,0 +1,23 @@
import { ResolverLoader, packageName } from '@embroider/core';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this back because i was getting duplicate packages, like glimmer-vm which does not like it

@@ -0,0 +1,59 @@
/* eslint-disable no-console */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to support ci tests with vite

if (!result) {
result = await context.resolve(
request.specifier,
request.fromFile.replace('/package.json', '/app/package.json'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to look into /app folder

Copy link
Contributor Author

@patricklx patricklx left a comment

Choose a reason for hiding this comment

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

@ef4 my current wip to virtualize generated assets. Am I om the right track?
149d5b7
Now i only need to make some files paths absolute instead of relative in the assets.
When this works i extract this into a separate pr.

@patricklx patricklx closed this Dec 22, 2023
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.

2 participants