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 find assets & build copy assets #1736

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Dec 19, 2023

  • resolves addon & app assets from their origin (not rewritten-app)
  • copies addons & apps public assets from origin (not rewritten-app)

@patricklx patricklx marked this pull request as draft December 19, 2023 19:06
@patricklx patricklx force-pushed the vite-copy-assets branch 2 times, most recently from 471f4a9 to f67133b Compare December 21, 2023 13:33
@patricklx patricklx marked this pull request as ready for review December 21, 2023 13:34
const publicAppFiles = glob.sync('**/*', {
cwd: pubDir,
});
for (const publicAppFile of publicAppFiles) {
Copy link
Contributor Author

@patricklx patricklx Dec 21, 2023

Choose a reason for hiding this comment

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

we do not copy the app assets that are available in app pkg meta.assets, because vite does bundle its own asset files. We only need the app public files

@patricklx patricklx changed the title vite copy assets to correct folder fux vite build Dec 21, 2023
@patricklx patricklx changed the title fux vite build fix vite build Dec 21, 2023
@patricklx patricklx force-pushed the vite-copy-assets branch 5 times, most recently from 03e3f3e to 9692e8f Compare December 22, 2023 09:53
packages/vite/src/hbs.ts Outdated Show resolved Hide resolved
@patricklx patricklx changed the title fix vite build vite build copy assets Feb 9, 2024
@patricklx patricklx force-pushed the vite-copy-assets branch 4 times, most recently from e2038c9 to e7b6465 Compare February 15, 2024 10:08
@patricklx patricklx changed the title vite build copy assets vite handle assets & build copy assets Mar 12, 2024
@patricklx patricklx changed the title vite handle assets & build copy assets vite find assets & build copy assets Mar 12, 2024
@ef4
Copy link
Contributor

ef4 commented Mar 12, 2024

Discussion notes from office hours:

  • let's use this.emitFile instead of manual file writing
  • let's use send in the middleware as in https://github.com/embroider-build/embroider/pull/1839/files. This avoids the node_modules remap.
  • let's try to let vite's stock public folder support handle the app, and our plugin only handles the addons (must ensure our plugin is installed after vite's)

@patricklx patricklx force-pushed the vite-copy-assets branch 4 times, most recently from e799ad3 to bb40dad Compare March 12, 2024 16:19
@patricklx patricklx force-pushed the vite-copy-assets branch 2 times, most recently from 2192ea9 to c335877 Compare March 19, 2024 14:14
@BlueCutOfficial
Copy link
Collaborator

BlueCutOfficial commented Mar 19, 2024

@patricklx what methodology did you use to test the PR? I noticed that it doesn't include the removal of the piece of code that writes the assets in the rewritten app.

@patricklx
Copy link
Contributor Author

@patricklx what methodology did you use to test the PR? I noticed that it doesn't include the removal of the piece of code that writes the assets in the rewritten app.

this is what i did

  • I remove compatPrebuild from vite config
  • pnpm ember b
  • manually delete assets from rewritten app
  • run vite build
  • check that assets are in dist

@patricklx
Copy link
Contributor Author

patricklx commented Mar 19, 2024

Maybe the code you changed also affects some package metadata? expecially public-assets?

@@ -66,7 +68,7 @@ export function assets(): Plugin {
this.emitFile({
type: 'asset',
source: readFileSync(join(pkg.root, path)),
fileName: dest.slice(1),
fileName: join('.', dest),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting, the result for join('.', './a-file') or join('.', '/a-file') is always 'a-file', nice

Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed the code to posix.resolve(‘/’, dest).slice(1) as suggested by Ed, it works as expected with my test setup (no problem in the middleware either) 👍

@ef4 ef4 merged commit e1142b1 into embroider-build:main Mar 28, 2024
93 checks passed
@BlueCutOfficial
Copy link
Collaborator

@patricklx for info tomorrow I'll work on the PR to clean-up the compat-app-builder :)

@mansona mansona added the enhancement New feature or request label Apr 4, 2024
@github-actions github-actions bot mentioned this pull request Apr 4, 2024
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants