Adding support for data uris in resolveUrl. #5199
Conversation
nicolasnoble
requested review from
dfreedm,
kevinpschaaf,
sorvell and
TimvdLippe
as code owners
| <script src="../../../webcomponentsjs/webcomponents-loader.js"></script> | ||
| <script src="../../../web-component-tester/browser.js"></script> | ||
| <link rel="import" href="../../polymer.html"> | ||
| <link rel="import" href="data:text/html,%3Cdom-module%20id%3D%22element-data-uri%22%3E%3Cscript%3EPolymer(%7Bis%3A%22element-data-uri%22%2Cproperties%3A%7Bexists%3A%7Btype%3ABoolean%2Cvalue%3Atrue%7D%7D%7D)%3C%2Fscript%3E%3C%2Fdom-module%3E"> |
There was a problem hiding this comment.
Would it be possible to put this into code with the URLencode? That would make the test more readable and understandable.
There was a problem hiding this comment.
Possibly, yes. I can try at least :-)
There was a problem hiding this comment.
Actually, what I attempted (visible here) works locally, but seems to timeout on tests on your travis environment. I'm not sure how to do this otherwise.
There was a problem hiding this comment.
Link to the failed travis build: https://travis-ci.org/Polymer/polymer/builds/367887168
nicolasnoble
force-pushed
the
data-uri
branch
2 times, most recently
from
b624a30
to
b7fddd7
Compare
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
I had another stab at it and it is indeed failing. I will need to run this on Sauce later to debug 😢 |
|
My guess is the onload trick isn't properly working everywhere for all browsers, but I am not sure how to solve it. Worst case scenario, for readability, it might be relevant to just add a comment next to the link that shows what its contents are. Also I would believe the test would be more relevant to use a Polymer component because that way it exercises the resolveURL codepath - your attempt doesn't. I can rearrange my original PR to add the comment block if you want. |
|
Yes lets resort to the comment. Strange thing is that it works just fine on
Firefox and Chrome locally, but on CI it borks.
…On Wed, 18 Apr 2018, 17:09 Nicolas Noble, ***@***.***> wrote:
My guess is the onload trick isn't properly working everywhere for all
browsers, but I am not sure how to solve it.
Worst case scenario, for readability, it might be relevant to just add a
comment next to the link that shows what its contents are.
Also I would believe the test would be more relevant to use a Polymer
component because that way it exercises the resolveURL codepath - your
attempt doesn't.
I can rearrange my original PR to add the comment block if you want.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#5199 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFrDb4rep8dZXMGHbWrEs-cjGMEXkgl7ks5tp1cugaJpZM4TZEtT>
.
|
|
CLAs look good, thanks! |
|
Yep, I have the same experience: it works fine locally for me too, so I don't exactly know what's going on with the CI... Oh well :-) I've updated the original PR to add a comment. PTAL. |
|
Sure. Is there going to be a planned Polymer 2.0 release with this fix possibly in there however ? I wouldn't like to have to upgrade my project immediately to Polymer 3.0 just to get this fix in there. |
|
Hm, I am actually not sure what happens in that case. I will discuss with the others and see what we can do! |
|
Okay, thanks. I mean, most of the time, substantials projects still get "maintenance fixes and releases" for a little while before abandoning the previous version, and Polymer 2.6.0 got released less than a month ago. It'd be quite harsh to drop support for it, forcing people to migrate to get fixes like this one in there. I mean, I could still monkey-patch my Polymer 2.6.0 during postinstall to incorporate this fix instead of waiting on a hypothetical 2.6.1, but I'd rather not. |
|
Yeah I am definitely not saying we are dropping support, we simply havent decided yet what our plans are. We still support 1.x with bug fixes, so we will for certain do that with 2.x as well. |
|
Cool, thanks! :) |
|
Gentle ping. I would actually love to see this one backported to the older polymer 2.x branch too. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Yeah it's stale alright. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Fixes #5198.