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

Fixes IE support #9

Merged
merged 1 commit into from Feb 11, 2019

Conversation

Projects
None yet
2 participants
@doanythingfordethklok
Copy link
Contributor

doanythingfordethklok commented Feb 8, 2019

The defensive check for node.js support causes IE to break. This MR updates File constructor test.

IE doesn't support the File constructor, but it does support the File API. In IE, typeof File evaluates to "object".

This broke our app when we upgraded to latest versions.

Fixes: #8

@evancz evancz merged commit e97c011 into elm:master Feb 11, 2019

@evancz

This comment has been minimized.

Copy link
Member

evancz commented Feb 11, 2019

IE!!!

Thanks for letting me know about this.

@doanythingfordethklok

This comment has been minimized.

Copy link
Contributor Author

doanythingfordethklok commented Feb 11, 2019

haha.. Actually, this one is probably more javascript's weird typeof construct combined with attempting to support node.js.

Anyhow.. I noticed that the debug library and a few other things do similar "does this type exists in the runtime" checks. If you'd like, I'll send a PR for debug and any other places where this check might be happening.

LMK if that'd be helpful.

@evancz

This comment has been minimized.

Copy link
Member

evancz commented Feb 11, 2019

Can you link to those spots here?

@doanythingfordethklok

This comment has been minimized.

Copy link
Contributor Author

doanythingfordethklok commented Feb 11, 2019

This one will make IE act differently than other browsers since IE implements the File spec, but does not expose a constructor. This is certainly low priority b/c folks shouldn't leave Debug in their production code.

https://github.com/elm/core/blob/master/src/Elm/Kernel/Debug.js#L151

@doanythingfordethklok

This comment has been minimized.

Copy link
Contributor Author

doanythingfordethklok commented Feb 11, 2019

I just went through everything in elm/core/Elm/Kernel and I think this is the only place with an issue. I opened IE And confirmed that DataView does implement the constructor in IE. Everything else in that folder looks bueno.

@evancz

This comment has been minimized.

Copy link
Member

evancz commented Feb 11, 2019

Thank you for the info on that. I'm hesitant to do patches on core for something like this (rather than bundling it with a bigger revision) but I'll go change it for now so that it'll get into whatever the next release is.

@doanythingfordethklok

This comment has been minimized.

Copy link
Contributor Author

doanythingfordethklok commented Feb 11, 2019

Agreed about patching core for this. The fix for file and json though are pretty critical for Elm users who need to select files and upload them. Since these quietly fail, it just doesn't work. I knew to dig into it b/c we knew it worked in the past and noticed it stopped working after upgrading from file@1.0.0 and json@1.1.1 to latest last Thursday.

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