-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support importing files as dataurl #107
Conversation
removed adding dataurl package as it could return a wrong value for mimetype, i also added a loader for svg |
the previous svg loader tries to load the file in utf-8, but somehow it didn't rendered properly when it's put to |
Thanks for figuring this out and sending a PR. I appreciate how small and simple the code ended up being. I'm trying to make the features I add to esbuild powerful and general-purpose, so I think for now it makes sense to just add the data URL loader and it doesn't make sense to special-case a single file extension like this (svg). What if you want to do this to other extensions as well? Or what if the mime type detector is wrong somehow? To me this speaks for the need of a more general mechanism. Perhaps |
yeah, the svg become problematic if we only use dataurl loader with i like the idea of configuring the mime type, it's powerful but quite verbose. so probably we need the ability to pass extension in |
I agree that passing in some information to the transform API would solve that problem. But in this case, using esbuild's transform API to do this transform seems like overkill to me. You could just do this to the same effect: function transform(svg) {
const base64 = Buffer.from(svg).toString('base64')
const text = `data:image/svg+xml;base64,${base64}`
return `module.exports = ${JSON.stringify(text)}`
} Why do you want to use esbuild for this? I'd expect using esbuild to actually be slower than those lines of JavaScript since the JavaScript code is pretty trivial and using esbuild would have to do cross-process communication. This PR seems mostly useful when you're using esbuild for bundling, in which case you already have the file extension. |
that's true, i now just added extension to mime type mapping that mimic extension to loader api so we could have escape hatch in case the mime type extension is wrong. this in turn make it usable in the transform api too |
to answer |
just a heads up, i will work on adding some more features (probably will open another PR based on this branch if this is accepted). right now i'm working to switch from webpack to esbuild in an electron app and the result is really good, especially on CI build and development reload time. the missing pieces are:
right now i'm using my published fork from this branch, with some changes on the app files to accommodate the lack of url loader and custom file name suffix resolving. but i hope we could reach some conclusion on the api and implementation so this could exists upstream :) i know that you need feedback more than contributions, but i'll be more than happy to discuss the api and contribute to the project too :) |
Thanks for the heads up. I'm glad that you're so excited about esbuild and willing to contribute, but I'm not ready for this level of contribution yet. I was willing to accept your initial PR because it was a small and targeted addition of a feature that I was planning to add anyway, but these changes are too wide in scope for me to be comfortable taking PRs for. This is why I've added this disclaimer in the readme:
It's very hard to remove features once they exist and people start building on them. I want to evolve esbuild forward thoughtfully and deliberately to try to avoid this as much as possible. This is especially important since there currently isn't an "escape hatch" of plugins where people can add their own requirements without impacting the user experience for everyone. I do of course eventually want esbuild to reach the state of being a full bundler with a lot of the features you mentioned. However, right now I'm pretty focused on the library use case. I want to make sure I get the fundamentals right (ES6 module output, code splitting, and tree shaking) before we start building a lot of stuff on top. That also lets esbuild start to be used in real production build pipelines even without all of the features that various real-world code bases use. |
I do understand that yeah, I'm already happy that i could have a fork of this code and do my changes on top of it, so i'm not worried if these contributions didn't end up upstream. :) as for this PR, the only changes I add from it's initial state is adding a new interface to set mime type + some more tests, do you want me to revert that changes? |
Yes, that would be helpful. I would gladly accept a PR with the original The other ideas are good but a bit too early I think. I want to be careful with the design of the loader API and don't want to add too much flexibility now and have people build on top of it, only to need to change it later. |
sure! the mime type mapping is reverted now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks great.
Hi! This is a great project, I noticed that it only supports base64 and text loader, i'm adding dataurl and svg support so we can import and use images / other assets right away in the bundle. This is improving the situation for #14 so the user does not need to create the dataurl themselves.
note: first time contributing to golang project, thank you for making the code so accessible!