search

embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
330 stars 137 forks source link

Hide base path from public URL of rollup-public-assets #1697

Closed simonihmig closed 5 months ago

simonihmig commented 6 months ago

This change is to enable a v2 addon to expose its public assets (using addon.publicAssets()) under the same URL as a v1 addon would. But currently, the public URL would always include the base path that you pass into the rollup plugin. Say if your v1 addon my-addon had public/foo/bar.jpg, it would expose taht under the URL of /my-addon/foo/bar.jpg. But if converting that to v2 and putting the file into e.g. src/public/foo/bar.jpg, then using the rollup plugin it would be exposed as /my-addon/src/public/foo/bar.jpg, and no way to get rid off the path (src/public).

This change is removing that, which makes this strictly speaking a breaking one though! 🤔

NullVoxPopuli commented 6 months ago

would if, instead, we add an option to options to allow transforming of the path? (like we do with app-js)?

This way, addon-authors could choose any URL scheme they want! :tada:

simonihmig commented 6 months ago

Yeah, we can do that. But instead or additionally? Like what should the plugin do by default, with the minimal usage of just passing the base path? Isn't what this PR is doing the better default?

If we are concerned about the breaking change though, having this option would certainly unblock me, and we could introduce that breaking change later when we have to do that anyway for some other reason. 🤔

mansona commented 1 month ago

So I have actually broken this PR in the latest Main because I merged stable which added this as an option: https://github.com/embroider-build/embroider/pull/1867

If we want to break it again then I guess we could invert the config so it would default to having no namespace but you could specify it? Thoughts?

mansona commented 1 month ago

I also removed the breaking label because the code has essentially been reverted now and it doesn't make sense to highlight it in the breaking changes

simonihmig commented 2 weeks ago

@mansona Sorry, missed your comments here... It seems that with #1874, we basically have the same changes back as in this PR, right? We just declared this a bug fix rather than a breaking change, is that fair to say? (I'm ok with that btw!)

simonihmig commented 2 weeks ago

Oh, missed the fact that that PR only removes the path when namespace is provided explicitly.

So my addon called fancy-button has publicAssets('src/public') and a file src/public/icon.svg, then this will make it available under the public URL of /fancy-button/src/public/icon.svg. Which is different from the behaviour a v1 addon would have. To not make this change in the public URL visible when migrating from v1 to v2, I would have to explicitly pass in the namespace: publicAssets('src/public', { namespace: 'fancy-button' ), right? It is the same namespace as the default, but now it would remove the extra path.

Tbh, I find this pretty confusing and unexpected. At least we would need to document this behaviour, as passing in the namespace has this unexpected side effect. But I would prefer the defaults to be good, which IMHO is to leave the path out either way and get back the v1 behaviour. Even if we need to mark this as a breaking change. Or we call it a bug fix, idk...

ef4 commented 1 week ago

From team discussion: we should get main back to the way @simonihmig's PR had it.