Allow references to files outside js/ and css/ dirs #26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows the files inside those other directories to be referenced from within JS/CSS files and properly bundled by esbuild.
Use an onLoad callback to track those references. This incorporates the approach from #27.
|
@krzykamil your idea is brilliant! I've just incorporated it into this PR. Could you please test things again on your app, using this branch? |
|
It looks fine, fonts are not duplicated and everything is correctly nested in my |
|
Just noting that using this branch with an app with slice assets fails to include the slice's JS/CSS files in the manifest. See this forum post for details. This is a bug and would be a regression. I'll address this before wrapping up this PR (and hopefully capture this in a test somehow). |
This can just return an array now. Also move it to the bottom of the file so it doesn’t get in the way of the more important things.
timriley
force-pushed
the
allow-references-to-files-outside-js-css-dirs
branch
from
0627aa2
to
3b29f91
Compare
timriley
changed the title
Call it extraAssetDirectories, which better reflects its purpose now, since we’re no longer marking any directories as “external”. Also, move this function to the bottom of the file to keep the mainline code easier to follow.
This means that if there is a “images/a/a.png” and a “images/b/b.png”, they’ll both have consistent manifest keys (“a/a.png” and “b/b.png” respectively), even in cases where esbuild is directly bundling one of the two files.
timriley
force-pushed
the
allow-references-to-files-outside-js-css-dirs
branch
from
bc46ffd
to
b53b416
Compare
Make this more consistent with esbuild’s name for this process.
This no longer seems necessary.
Make it clearer that the “path” is just the “dir” but a full path.
Make it clearer how we handle these in full by copying the assets and then populating their entries in the manifest immediately after.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stop marking the directories aside from js/ and css/ as "external". This allows the files inside those other directories to be referenced from within JS/CSS files and properly bundled by esbuild.
This incorporates @krzykamil's (excellent!) suggestion in #27, which uses an
onLoadcallback to track referenced files so that we can exclude them from separate handling when we copy over the non-referenced asset files.Specific changes:
esbuild.js)findExternalDirectoriesfromesbuild.jstoesbuild-plugin.jsasextraAssetDirs(a more appropriate name now that nothing is marked as external), and call this directly when determining which directories we should manually copy viaprocessAssetDirectory.onLoadcallback toesbuild-plugin.jsand use this to track any files that esbuild itself loads.processAssetDirectory, which means there will be no double-processing of the files that are referenced from JS/CSS.processAssetDirectory.Thanks also to @svoop for the great initial bug report (in #24 as well as the ensuing forum conversation)!
Fixes #24.