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

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

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

simonihmig
Copy link
Collaborator

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! 🤔

This change is to enable a v2 addon to expose its public assets 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 a breaking one.
@simonihmig simonihmig requested a review from a team December 4, 2023 18:06
@NullVoxPopuli
Copy link
Collaborator

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! 🎉

@simonihmig
Copy link
Collaborator Author

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. 🤔

@ef4 ef4 merged commit 0dc2c93 into main Dec 11, 2023
201 checks passed
@ef4 ef4 deleted the public-assets-hide-base-path branch December 11, 2023 12:22
@github-actions github-actions bot mentioned this pull request Dec 11, 2023
@github-actions github-actions bot mentioned this pull request Apr 4, 2024
@mansona
Copy link
Member

mansona commented Apr 10, 2024

So I have actually broken this PR in the latest Main because I merged stable which added this as an option: #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 mansona added internal and removed breaking labels Apr 10, 2024
@mansona
Copy link
Member

mansona commented Apr 10, 2024

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

@github-actions github-actions bot mentioned this pull request Apr 10, 2024
@simonihmig
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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
Copy link
Contributor

ef4 commented May 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants