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

Added 'use-credentials' Cors Option to ImageOverlay and TileLayer #6016

Merged
merged 3 commits into from
Jan 23, 2018
Merged

Added 'use-credentials' Cors Option to ImageOverlay and TileLayer #6016

merged 3 commits into from
Jan 23, 2018

Conversation

caleblogan
Copy link
Contributor

Fixes #5891

I added a new option to set the crossOrigin property to 'use-credentials'. Another solution would be to make crossOrigin a String and set the default value to null. Let the user set crossOrigin as a String.

@ghybs
Copy link
Collaborator

ghybs commented Jan 19, 2018

Hi,

Thank you for contributing your first PR here!

I like that this new "crossOriginCredentials" option would make it easy to set the crossorigin="use-credentials" attribute and value.

Unfortunately it may conflict with the already existing crossOrigin option; e.g. if we have:

{
  crossOrigin: true,
  crossOriginCredentials: true
}

…the <img> will have the crossorigin="use-credentials" attribute and value (with current state of PR), although with crossOrigin: true we have requested to get crossorigin="".

Furthermore, there are actually 3 possible states for this attribute:

  • crossorigin="use-credentials" => "cors" mode and "include" credentials mode.
  • crossorigin="anonymous" (or actually any other value than "use-credentials") => "cors" mode and "same-origin" credentials mode.
  • no/omitted crossorigin attribute => "no-cors" mode.

Therefore we need at least a tristate option.

Another solution would be to make crossOrigin a String

That would indeed match with @IvanSanchez's #5891 (comment)

We could have:

  • type "string" => crossorigin="<crossOrigin-option-value>"
  • other truthy value => crossorigin="" (same effect as crossorigin="anonymous")
  • false (default) => no crossorigin attribute ("no-cors" requests mode)

This scheme would require the app developer to explicitly specify crossOrigin: 'use-credentials' option (instead of just crossOriginCredentials: true), but it leverages the already existing crossOrigin option and maintains backwards compatibility.

It also leaves this string out of the library (saves a few bytes for common case…), and keeps the door open to any other keyword that may appear in the future (how unlikely this may sound).

@IvanSanchez @cherniavskii what do you think?

@cherniavskii
Copy link
Collaborator

cherniavskii commented Jan 19, 2018

@ghybs I like proposition of crossOrigin accepting string - it's obvious and flexible.
Also, it's easy to refer crossorigin attribute docs in our documentation :)

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about reusing the crossOrigin option for that? We could just use its value for crossOrigin if a string is supplied, otherwise set to '' if truthy.

@caleblogan
Copy link
Contributor Author

caleblogan commented Jan 20, 2018

I wasn't too sure how to phrase the docs. Especially the default value, String = false. I didn't do any type checking so setting crossOrigin=True will result in <img crossorigin="true"/>. This will still work, but I am not sure if it could cause a possible backwards compatibility problem.

@ghybs
Copy link
Collaborator

ghybs commented Jan 22, 2018

Thank you for the modification to make the PR now re-use the crossOrigin option! 👍

Even though the spec says that an invalid keyword (as "true" in this case) is interpreted as "anonymous" and "" (empty string), it does not feel right that a currently valid option crossOrigin: true would then lead to using an invalid keyword.

Therefore I think we should at least convert crossOrigin: true to crossorigin="".

As for the docs, you should specify that a Boolean type is possible, using a vertical pipe (|), like is done for the doubleClickZoom map option for example:

// @option doubleClickZoom: Boolean|String = true
// Whether the map can be zoomed in by double clicking on it and
// zoomed out by double clicking while holding shift. If passed
// `'center'`, double-click zoom will zoom to the center of the
// view regardless of where the mouse was.
doubleClickZoom: true

Copy link
Collaborator

@ghybs ghybs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you four the modifications!

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

Successfully merging this pull request may close these issues.

4 participants