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

Use esbuild metafile. Add support for 'entryNames' esbuild option. #678

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

into-the-v0id
Copy link
Contributor

@into-the-v0id into-the-v0id commented Oct 19, 2024

Description

Currently (before this PR) the esbuild plugin would always output JS asset files as options.outdir + replaceExtension(page.data.url). Which means The esbuild option entryNames would not work.

Additionally the esbuild plugin tries to map output files from esbuild to lume pages (JS eintry points). This looks similar to this:

const page = pages.find((page) => withoutExtension(page.sourcePath) === withoutExtension(outfile.path));

When entryPoints is set/changed this no longer works. The plugin then can not find a page for the outfile and adds it as a chunk.

When using the entryNames option like this:

site.use(esbuild({
  options: {
    entryNames: "js/[name].[hash]",
  },
}));

Given a source file scripts/index.ts you would get something like the following output files:

  • js/index.WSRTVQIK.js
    • No src connection
    • Bundled and transformed file
  • scripts/index.ts
    • Src connection: scripts/index.ts
    • No transforms - raw source file

This PR adds support for the entryPoints option. It now uses the esbuild metafile which contains information about input and output files and their relation. Using this metafile we can properly map esbuild outfiles to lume pages. It now also uses the esbuild outfile filename as final data.url:

page.data.url = outfile.path.replaceAll(
  dirname(page.sourcePath),
  dirname(page.data.url),
)

The replace is there to support url overwrites via basename. Using lume url overwrites in combination with the entryNames esbuild option works generally. However as we only write the file to a different location to what esbuild intended, JS imports to this file (e.g. via dynamic imports) are not rewritten! Esbuild currently does not offer any functionality to dynamically rewrite an outfile path per file (as far as I know). So this is as much as we can do.

I have also mostly reverted the changes from commit 13796a7 as that workaround is no longer necessary.

Related Issues

None

Check List

  • Have you read the
    CODE OF CONDUCT
  • Have you read the document
    CONTRIBUTING
    • One pull request per feature. If you want to do more than one thing,
      send multiple pull request.
    • Write tests.
    • Run deno fmt to fix the code format before commit.
    • Document any change in the CHANGELOG.md.

into-the-v0id added a commit to into-the-v0id/lume that referenced this pull request Oct 19, 2024
… files to pages. This adds support for the 'entryNames' option of esbuild lumeland#678
@into-the-v0id into-the-v0id force-pushed the bugfix/esbuild-outfiles branch from e87ed9f to 40a7029 Compare October 19, 2024 18:52
into-the-v0id added a commit to into-the-v0id/lume that referenced this pull request Oct 19, 2024
… files to pages. This adds support for the 'entryNames' option of esbuild lumeland#678
@into-the-v0id into-the-v0id force-pushed the bugfix/esbuild-outfiles branch from 40a7029 to e835ec5 Compare October 19, 2024 18:55
into-the-v0id added a commit to into-the-v0id/lume that referenced this pull request Oct 19, 2024
… files to pages. This adds support for the 'entryNames' option of esbuild lumeland#678
@into-the-v0id into-the-v0id force-pushed the bugfix/esbuild-outfiles branch from e835ec5 to 7961d31 Compare October 19, 2024 19:04
Copy link
Member

@oscarotero oscarotero 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 this! I didn't know the metafile option but it seems very useful!
I just left some comments in the review, some of them are optional. To me, the most sensible part is the change of the output directory, which can break the code.

plugins/esbuild.ts Outdated Show resolved Hide resolved
plugins/esbuild.ts Show resolved Hide resolved
plugins/esbuild.ts Outdated Show resolved Hide resolved
plugins/esbuild.ts Show resolved Hide resolved
into-the-v0id added a commit to into-the-v0id/lume that referenced this pull request Oct 20, 2024
… files to pages. This adds support for the 'entryNames' option of esbuild lumeland#678
@into-the-v0id into-the-v0id force-pushed the bugfix/esbuild-outfiles branch from 7961d31 to 10e7353 Compare October 20, 2024 18:28
… files to pages. This adds support for the 'entryNames' option of esbuild lumeland#678
@into-the-v0id into-the-v0id force-pushed the bugfix/esbuild-outfiles branch from 10e7353 to 8620265 Compare October 20, 2024 18:29
@into-the-v0id
Copy link
Contributor Author

To me, the most sensible part is the change of the output directory, which can break the code.

Note that the previous implementation completely ignored the esbuild output path and just wrote it to page.data.url - which also breaks dynamic imports if we use lume url overwrites via basename.

@oscarotero
Copy link
Member

Note that the previous implementation completely ignored the esbuild output path and just wrote it to page.data.url - which also breaks dynamic imports if we use lume url overwrites via basename.

Yeah, to be honest, I hadn't thought about that situation until now! :)
Thanks for your contribution. The esbuild plugin is much stronger now!

@oscarotero oscarotero merged commit 9fa30af into lumeland:main Oct 20, 2024
6 checks passed
@into-the-v0id into-the-v0id deleted the bugfix/esbuild-outfiles branch October 20, 2024 20:04
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