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

feat: Resolve relative URLs in property values #18

Merged
merged 2 commits into from
Mar 1, 2020
Merged

feat: Resolve relative URLs in property values #18

merged 2 commits into from
Mar 1, 2020

Conversation

alex-ketch
Copy link
Contributor

Hi @unlight, and thanks for this plugin!

I encountered a need for converting relative paths referenced in remote files to be absolute, as otherwise it the relative files would fail to resolve, and cause build issues down the pipeline.
Let me know if this PR is something you'd want to accept or not.
But for our use case, resolving assets paths to be absolute allows for chaining with other plugins to either download the assets or inline them as base64.

I've added tests which should illustrate the resolution better, but here's the idea:

/* myFile.css */
@import url("https://example.com/styles.css");
/* https://example.com/styles.css */
@font-family {
  src: url("./relative/path/font.woff");
}

Output

/* myFile-compiled.css */
@font-family {
-  src: url("./relative/path/font.woff");
+  src: url("https://example.com/relative/path/font.woff");
}

One question though, I didn't put this functionality behind an option flag as I didn't think keeping asset paths relative would be a desirable very often, but I can see how that might be misconception on my part, or even a breaking change for some setups.
I'd appreciate your guidance on how, and if, you'd like this to work.

Thanks again 👋

@alex-ketch alex-ketch requested a review from unlight February 20, 2020 03:26
@unlight
Copy link
Owner

unlight commented Feb 20, 2020

@unlight
Copy link
Owner

unlight commented Feb 25, 2020

@alex-ketch ping

@alex-ketch
Copy link
Contributor Author

Hi @unlight, I've been busy with work but will try to to confirm what I encountered by making a test repo.

But from I remember, I tried using (postcss-url)[https://github.com/postcss/postcss-url] but it did not solve my problem because it was still looking for the resources on my local machine, rather than on the remote server. I did not come across https://github.com/devex-web-frontend/postcss-assets-rebase during my searches.
Will get back to you once I do a bit more testing.

Thanks for your patience

@alex-ketch
Copy link
Contributor Author

Hi again @unlight, I've made a repo with a minimal setup demonstrating the issue this PR tries to solve. I also have a couple configurations using the plugins you suggested, but they don‘t seem to solve the problem either.
It can be found here: https://github.com/alex-ketch/postcss-import-url-resolve-test

Let me know if you have any questions are need clarification.
Thanks

@unlight
Copy link
Owner

unlight commented Feb 26, 2020

@alex-ketch
PR looks good to me (I'm going to accept next weekend), but it would be nice if new feature would be hidden under option with default value false.

@alex-ketch
Copy link
Contributor Author

Appreciate it @unlight, I can put it behind a flag.
Does either resolveUrls or qualifyUrls sound good for the option name? Preference over singular vs plural?

@unlight
Copy link
Owner

unlight commented Feb 26, 2020

resolveUrls sounds good (plural).

@alex-ketch
Copy link
Contributor Author

@unlight I've made the changes, added test, and update the README.
Let me know if you have any feedback

@unlight unlight merged commit 4d04c97 into unlight:master Mar 1, 2020
@unlight
Copy link
Owner

unlight commented Mar 1, 2020

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants