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

Support importing files as dataurl #107

Merged
merged 19 commits into from
May 22, 2020
Merged

Conversation

viankakrisna
Copy link
Contributor

@viankakrisna viankakrisna commented May 12, 2020

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!

@viankakrisna viankakrisna changed the title add dataurl loader support Support importing file as dataurl May 12, 2020
@viankakrisna viankakrisna changed the title Support importing file as dataurl Support importing svg and other file as dataurl May 13, 2020
@viankakrisna
Copy link
Contributor Author

removed adding dataurl package as it could return a wrong value for mimetype, i also added a loader for svg

@viankakrisna
Copy link
Contributor Author

viankakrisna commented May 13, 2020

the previous svg loader tries to load the file in utf-8, but somehow it didn't rendered properly when it's put to <object> tag, i kept svg loader as the faster path for dataurl instead, it also makes it easier to run it on js service mode

@evanw
Copy link
Owner

evanw commented May 13, 2020

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 --loader:.svg=dataurl?mime=image/svg+xml? I'll have to think about that one.

@viankakrisna
Copy link
Contributor Author

viankakrisna commented May 14, 2020

yeah, the svg become problematic if we only use dataurl loader with service.transform api, as we don't have the extension information. in this case, the extension becomes .dataurl, which will have us kick into this line https://github.com/evanw/esbuild/pull/107/files#diff-c1ea173c14bc907084c0fa137e6795caR112 that's my reasoning on extracting svg as a separate loader, but it's probably short sighted 😅

i like the idea of configuring the mime type, it's powerful but quite verbose. mime.TypeByExtension(extension) is good on figuring out the actual mime type based on extension. http.DetectContentType is hit and miss.

so probably we need the ability to pass extension in service.transform too?

@evanw
Copy link
Owner

evanw commented May 14, 2020

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.

@viankakrisna
Copy link
Contributor Author

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

@viankakrisna
Copy link
Contributor Author

viankakrisna commented May 14, 2020

to answer Why do you want to use esbuild for this?: the transform api is just what i did for testing the feature 😅 on my app i use the esbuild.build api, i now added some more test in the actual place where this would be useful

@viankakrisna
Copy link
Contributor Author

viankakrisna commented May 14, 2020

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:

  1. dataurl loader (we inline assets so we don't need to wait for io to show image) this is working fine with the branch, although we probably should fallback to url loader once the file size reach some defined number, this is affecting the app boot time.
  2. url loader (for including css and big files, i actually want to work on this first but dataurl is easier to implement as it does not need to emit file, but i think it could be done with addition of publicPath in the api)
  3. custom file name suffix resolving (something like react native) this is possible in webpack but really verbose, i'm thinking of adding something like preferSuffix: '.desktop' so it loads someFile.desktop.js when it exists instead of someFile.js
  4. @babel/register like functionality so we don't need to use babel anymore in our tests too
  5. some entry point -> html attachment, this is possible in jsland but i think the cli should have it too
  6. minify other files (svg,html,css) not sure if these should be reimplemented or https://github.com/tdewolff/minify is good for it

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 :)

@evanw
Copy link
Owner

evanw commented May 14, 2020

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:

I'm mainly looking for feedback at the moment, not contributions. The project is early and I'm still working toward an MVP bundler that can reasonably replace real-world toolchains. There are still major fundamental pieces that haven't been put in place yet (e.g. CSS support, watch mode, tree shaking, code splitting) and they all need to work well together to have best-in-class performance.

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.

@viankakrisna
Copy link
Contributor Author

viankakrisna commented May 14, 2020

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?

@evanw
Copy link
Owner

evanw commented May 21, 2020

do you want me to revert that changes?

Yes, that would be helpful. I would gladly accept a PR with the original dataurl loader.

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.

@viankakrisna
Copy link
Contributor Author

sure! the mime type mapping is reverted now.

@viankakrisna viankakrisna changed the title Support importing svg and other file as dataurl Support importing files as dataurl May 21, 2020
Copy link
Owner

@evanw evanw left a 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.

@evanw evanw merged commit eb8f4f1 into evanw:master May 22, 2020
@evanw evanw mentioned this pull request Jun 20, 2020
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