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

Rename crossorigin to crossOrigin for greater compatibility with Image #967

Open
codebycarlos opened this issue Oct 12, 2022 · 7 comments
Open

Comments

@codebycarlos
Copy link

codebycarlos commented Oct 12, 2022

First of all thank you so much for this package, it's pretty awesome!

If we want to make the api fully compatible with the regular image element we'll need to make the component recognise when users pass in the crossOrigin prop as opposed to just crossorigin. With regular image elements in React, you'd need to pass in the attribute with the crossOrigin casing to avoid any lint errors.

image

Current setup can lead to confusing behaviour when one sets the crossOrigin prop but it is then ignored by react-image (happened to me, probably pretty rare as most people don't use this attribute). I fixed this by passing in the defined crossorigin prop, but either way it should accept both ideally.

@codebycarlos codebycarlos changed the title make crossorigin prop case insensitive add support for crossOrigin prop Oct 12, 2022
@mbrevda
Copy link
Owner

mbrevda commented Oct 13, 2022

Unclear on the issue here: is the crossorigin prop being set on the <img> element? Is this purely a lint issue?

@codebycarlos
Copy link
Author

codebycarlos commented Oct 13, 2022

Unclear on the issue here: is the crossorigin prop being set on the <img> element? Is this purely a lint issue?

So the main issue is when you pass the crossOrigin prop to the <Img /> component from 'react-image'; it gets ignored (you can see here that only crossorigin is recognised and passed to the imagePromiseFactory function).

This behaviour is different to the native <img /> element which accepts crossOrigin (trying to pass crossorigin works too but triggers a lint warning in React). Very minor issue, I know, but #972 should resolve it.

@mbrevda
Copy link
Owner

mbrevda commented Oct 13, 2022

Any props not used by the Img component will be passed through to the underlying <img> element as HTML attributes, as can be seen here and here. I'm not sure why we would have to special case this attribute vs any other valid HTML attribute?

@codebycarlos
Copy link
Author

codebycarlos commented Oct 13, 2022

Any props not used by the Img component will be passed through to the underlying <img> element as HTML attributes, as can be seen here and here. I'm not sure why we would have to special case this attribute vs any other valid HTML attribute?

That is correct, however, in the special case of crossorigin and requesting of the image, this is first handled by imagePromiseFactory.jsx via the useImage hook here.

By the time the image src and remaining image props are passed to the <img /> element you mention, crossorigin doesn't make a difference as the image was already requested and cached according to the crossorigin passed (i.e. with Origin header or not depending on its value).

Basically, the way I noticed this was even an issue when using 'react-image' was this: my team had a situation where we needed to set an image's crossorigin to 'anonymous' (otherwise using it within a canvas would fail). Initially, I did something like:

   <Img crossOrigin="anonymous"/>

Yet the image was still failing, which led me to discover <Img /> from 'react-image' was not picking up crossOrigin, and only crossorigin (unlike a native <img /> element). Upon switching it to

   <Img crossorigin="anonymous" />

everything started working again 🎉

@mbrevda
Copy link
Owner

mbrevda commented Oct 13, 2022

So the main issue is when you pass the crossOrigin ... it gets ignored

Not really following why this is an issue - the componenet seems to be working as intended?

This behaviour is different to the native element which accepts crossOrigin

Oh - you're suggesting that the prop be named in a manner consistent with the native element?

@codebycarlos
Copy link
Author

codebycarlos commented Oct 13, 2022

It works as intended, but only when using 'crossorigin'. With 'crossOrigin', it's as if you had not passed it in at all, as crossorigin is what gets used to make the image request at first (crossorigin is important when it comes to this as it determines CORS behaviour). So while crossOrigin (and remaining imgProps) do get passed to the image element later on, as you mentioned, by that point it's too late if the browser has already cached the image (particularily an issue with Chrome/Safari).

This may catch people off guard switching their codebase from the native <img /> element to react-image's as was the case with my team.

Native:

   <img crossorigin="use-credentials" /> // works ✅
   <img crossOrigin="use-credentials" /> // works ✅

React-Image:

   <Img crossorigin="use-credentials" /> // works ✅
   <Img crossOrigin="use-credentials" /> // doesn't work ❌

I'm suggesting we support both, crossorigin (currently supported) and crossOrigin (not currently supported). Sorry, I know the crossorigin attribute is quite a confusing one altogether, I'm trying my best to explain.. 😅

@mbrevda
Copy link
Owner

mbrevda commented Oct 13, 2022

Got it. Can consider remaining the prop in an upcoming major version

@mbrevda mbrevda changed the title add support for crossOrigin prop Rename crossorigin to crossOrigin for greater compatibility with Image Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants