Comments (10)
Which makes me think that paths without extensions should be skipped, which effectively means ignoring directories (although a path without an extension can be a file too).
And a path with an extension could also be a directory. IMHO one has to look into the file system, and then
- if the path represents a file => emit that file as an asset
- if the path represents a directory => ignore it
- if nothing exists at the path => log an error or warning, depending on
warnOnError
My pull request makes the plugin behave like that.
from web.
Patterns that reference directories should be silently ignored by default.
Why should they be ignored and not resolved to a correct dir path?
I'm thinking of a use-case where one might resolve a dir once and then construct paths to images within this dir.
Am I missing smth in regard to how directories are different in the path resolution algorithm?
from web.
If patterns can be resolved also to a dir path then that would be even better considering the use case that you mentioned (currently such patterns produce an error/a warning but are not resolved).
I think it depends on how one sees the scope of the plugin. When I made the pull request, I had a narrower scope in mind: if there is an asset then the plugin should bundle it, otherwise do nothing. That was my impression when reading the docs.
OTOH, one could think of a wider scope such as:
- if there is a dir or file path then resolve it
- if it is a file path then consider that file an asset and bundle it
- if the dir/file does not exist at build time then raise an error/a warning
From my point of view, both options are agreeable, but the second one is more appealing. Please let me know how you would like to proceed -- thanks!
from web.
@bashmish Sorry, my fingers were faster than my brain...
On second thought, resolving a dir path does not make sense; what should it be resolved to? There is no corresponding asset. So please disregard the second option I mentioned above.
from web.
resolving a dir path does not make sense
@undecaf do you mean it doesn't make sense in the context of assets or in general?
I agree that the scope of this plugin should focus on assets only. But I struggle to understand what the use case for new URL('./', import.meta.url)
is? Is it not for assets? Can you explain why you have it in your code?
from web.
do you mean it doesn't make sense in the context of assets or in general?
@bashmish In the context of assets (i.e. like new URL('./img/picture.jpg', import.meta.url)
) it should of course be resolved to the path of that file in the bundle.
But resolving resolve something like e.g. new URL('./', import.meta.url)
(i.e. replacing it with the path at which the module will be located after bundling) IMHO is not necessary; at runtime this statement will return that path anyway. The same holds true for relative subpaths such as new URL('./img/', import.meta.url)
.
from web.
But I struggle to understand what the use case for new URL('./', import.meta.url) is? Is it not for assets? Can you explain why you have it in your code?
@bashmish new URL('./', import.meta.url)
appears in the code that Emscripten generates for loading WebAssembly files. It is used to locate the WebAssembly file at runtime. As I mentioned above, when bundled this works without having been resolved.
from web.
Thanks for explaining the use-case, it makes it much more clear.
I think it's not about directories though. The tools might include other paths that need to be resolved in runtime only.
Instead of ignoring all directory paths I'd say you should pin-point the ones that can be ignored.
Can you use exclude
option to disable the plugin for a specific path where this code appears?
https://modern-web.dev/docs/building/rollup-plugin-import-meta-assets/#exclude
If not, maybe we need another config to exclude
or ignore
specific files/paths/urls.
from web.
Instead of ignoring all directory paths I'd say you should pin-point the ones that can be ignored.
If warnOnError
is true
, the current version of the plugin already ignores all paths that reference a directory, i.e. they appear in the bundle as-is. If warnOnError
is false
then the build fails if a directory path is found.
Therefore my pull request does not change the bundling result compared to what the current plugin version produces; it just silences the directory warning (if warnOnError
is true
) and lets the build succeed (if warnOnError
is false
).
Can you use exclude option to disable the plugin for a specific path where this code appears?
I am the author of a package which contains statements like new URL('./', import.meta.url)
(referencing a directory) but also new URL('./zbar.wasm', import.meta.url)
(referencing an asset) in the same module. Therefore exclude
would also prevent the asset from being bundled.
Besides, as that package is a library, all consumers of that package would have to configure the Rollup plugin with exclude
. I would prefer to not impose any particular configuration option on the consumers of the package. BTW not having to set warnOnError: true
as package consumer was the reason why I submitted this pull request.
from web.
I am the author of a package which contains statements like new URL('./', import.meta.url) (referencing a directory) but also new URL('./zbar.wasm', import.meta.url) (referencing an asset) in the same module. Therefore exclude would also prevent the asset from being bundled.
Then indeed current exclude
API won't work.
I myself ran into similar difficulties with another plugin @web/rollup-plugin-html
(https://github.com/bashmish/storybook-builder-wds/blob/0.1.0/src/index.ts#L142) when some of the assets could't not be resolved in the index.html
, because they were supposed to be served by proxy later. I used extractAssets: false,
which worked in my case, although I had to change some code.
This is another plugin and another use-case, but very similar. After that on the back of my head I remembered that we might need to add more fine-grained control over specific assets that we are processing. That's why I was hesitating to make assumptions like "all directory paths should be ignored".
But I also checked the file https://github.com/modernweb-dev/web/blob/master/packages/rollup-plugin-import-meta-assets/src/rollup-plugin-import-meta-assets.js and see that we say:
Checks if a AST node represents this kind of expression:
new URL('./path/to/asset.ext', import.meta.url)
And in docs (https://github.com/modernweb-dev/web/blob/master/docs/docs/building/rollup-plugin-import-meta-assets.md) we say:
Rollup plugin that detects assets references relative to modules using patterns such as new
URL('./assets/my-img.png', import.meta.url)
.
Which makes me think that paths without extensions should be skipped, which effectively means ignoring directories (although a path without an extension can be a file too).
I'll review your PR now.
from web.
Related Issues (20)
- [test-runner-playwright] unable to use msedge browser HOT 1
- Summary report no longer outputs summary of errors HOT 1
- [dev server] appIndex ignored if the url is not a sub-directory of the html file
- Adding a response header to Dev server seems excessively convoluted, am I missing something? HOT 4
- Storybook docs do not explain how to view the storybook. HOT 1
- [blog post] Write about the JavaScript module mess
- on playwright firefox, sendmouse is flaky with concurrency > 1 HOT 1
- Getting error when reading some data Invalid UTF-8 leading byte 0x000000bd encountered when deserializing a UTF-8 string in wasm memory to a JS string HOT 1
- Named import from CommonJS module is broken HOT 2
- Broken Code Coverage Results
- [rollup-plugin-html] does not extract assets within `<template>` tags
- [mocks] fails to load service work on subfolders HOT 3
- [rollup-plugin-import-meta-assets] Support string expressions in URL call or document string literals only HOT 2
- [test-runner] Setting concurrency > 1 using chrome launcher can hang and timeout on Ubuntu HOT 1
- [dev-server-storybook] v1 published files are wrong format HOT 26
- Using playwright reporters when testing with playwright HOT 1
- Allow to override nodeResolve exportConditions development in the dev server HOT 2
- Remove the localhost conversion, since it prevents easy secure origin based feature tests
- ChromeLauncherPage always sets viewport to 800 x 600 ignoring createPage config
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from web.