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

IE / Edge #16

Closed
testower opened this issue Oct 10, 2017 · 14 comments
Closed

IE / Edge #16

testower opened this issue Oct 10, 2017 · 14 comments
Assignees
Labels

Comments

@testower
Copy link

Expected behavior

According to docs, this component should work with IE11 with a polyfill, and with Edge 15 without a polyfill.

Current behavior

I am unable to even get the storybook example to work in Edge 15. The onChange action logger reports undefined for "isIntersecting".

Steps to reproduce

  1. Open the storybook example in Edge 15
  2. Observe that the example doesn't work
  • Version: 0.4.2
  • Platform: Browserstack + Edge 15
@danez danez added the bug label Oct 10, 2017
@Rendez Rendez self-assigned this Oct 10, 2017
@Rendez
Copy link
Contributor

Rendez commented Oct 10, 2017

@testower thanks for your detailed report. Unfortunately, I will only have access to an Edge browser tomorrow morning.

In the meantime, please double-check you have really installed the version 0.4.2 of the polyfill; as you probably already know, some support for Edge 15 was added not long ago.

Also note that while this issue is fixed on Edge, hasn't been shipped just yet: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12156111/

In any case, we'll release any relevant changes within a day's time.

@testower
Copy link
Author

Cheers, the version I referred to above was the polyfill version, sorry about that.

Would that polyfill not fix the issue on Edge meanwhile?

Also, I seem to have a hard time in IE11, on my end, but unable to give steps at this point. It seems to me that the onChange doesn't fire at all.

@Rendez
Copy link
Contributor

Rendez commented Oct 10, 2017

It struck me a moment ago that I did not include the polyfill in the storybook's JS bundle. I will fix this tomorrow. Now, this only affects the storybook example page and including the polyfill in your codebase as usual should work as expected, and trigger the onChange callbacks even in IE11.

To be concise for others landing on this issue: this is a bug only within the examples and will be fixed with the next release.

@testower
Copy link
Author

testower commented Oct 11, 2017

The reason I opened the bug is that I can't get react-intersection-observer to work in IE11 or Edge, even with the polyfill loaded.

Am I correct in understanding that it should work in Edge despite the open issue there, because the polyfill will handle it?

Then why it doesn't work in IE11 I'm not sure. It seems like the onChange handler isn't called. I will try and find a reproducible case.

Edit: I know the polyfill is loaded, because it works fine in Safari.

Edit2: So I guess there's something wrong with my implementation. I now find that the onChange isn't fired in Edge either, with or without polyfill. We would expect it to at least fire, but with undefined isIntersecting property when there's no polyfill. I'm using an HOC based on your example:

import React, { Component } from 'react';
import _defer from 'lodash/function/defer';
import 'intersection-observer';
import Observer from '@researchgate/react-intersection-observer';

export default (options) => (WrappedComponent) => {
  const {
    root = null,
    rootMargin = '0px 0px 0px 0px',
    threshold = 0,
    onlyOnce = false,
    disable = false
  } = options;

  return class WithIntersectionObserver extends Component {
    constructor(props) {
      super(props);

      this.state = {
        isIntersecting: false
      };

      this.onChange = this.onChange.bind(this);
    }

    onChange({ isIntersecting, intersectionRatio }) {
      this.setState({
        isIntersecting: isIntersecting &&
          intersectionRatio >= threshold
        });
    }

    render() {
      const deferredChangeHandler = (entry) => {
        console.log('deferredChangeHandler', entry); // never printed in console
        _defer(() => {
          this.onChange(entry);
        });
      };

      return (
        <Observer
          root={root}
          rootMargin={rootMargin}
          threshold={threshold}
          onlyOnce={onlyOnce}
          disable={disable}
          onChange={deferredChangeHandler}>
          <WrappedComponent
            {...this.props}
            isVisible={this.state.isIntersecting} />
        </Observer>
      );
    }
  };
};

I'm using this in an export of another component:

export default withIntersectionObserver({
  rootMargin: '0px 0px 50px',
  onlyOnce: true
})(SomeComponent);

As I understand it, this should create an intersection observer on the window/viewport, since that's the default behavior when root is null or omitted. I've tried to put it on a wrapper element but doesn't make a difference.

@Rendez
Copy link
Contributor

Rendez commented Oct 11, 2017

I've released a new version which includes the necessary polyfills for storybook. I tested using Browserstack in both browsers, locally and on the deployed storybook on Github pages without problems. Let me know if you're still having issues with it.

@Rendez
Copy link
Contributor

Rendez commented Oct 12, 2017

@testower I am closing this bug since the issue with the polyfill is resolved on the examples page. If you're having issues with the snippet you provided, please create an example using codesandbox using the v0.4.0 version, and share it with us.

@Rendez Rendez closed this as completed Oct 12, 2017
@testower
Copy link
Author

Cheers, This is the minimal example I have that fails in Edge:

https://codesandbox.io/s/3vv2nw8zvm

Possibly an issue with the HOC, but I can't figure out what exactly. As far as I can tell, the onChange provided isn't fired.

@Rendez
Copy link
Contributor

Rendez commented Oct 13, 2017

I've reviewed your example. Your empty DIVs happen to be zero px height. Older implementations of Chrome, and Edge 14/15 I believe, exhibit this old behavior. The spec was changed a while back to match edge-adjacent intersections (matching the 0 pixel DIV), which would fire events.

You can fix your example with:

return (
  <div style={{ minHeight: 1 }}>
    {content}
  </div>
);

Let me know if this fixes your issue.

@testower
Copy link
Author

Thanks! I had completely ignored the logic that involves the intersection ratio, but it makes total sense to only care about isIntersecting in my implementation.

So taking care of that and the minHeight fixes the Edge issue. But I still can't get IE11 to work. Will try and get something reproducable, but struggling to get even codesandbox to load there.

@testower
Copy link
Author

testower commented Oct 14, 2017

Oh, I suppose this is due to:

Support for _Map_, _Symbol_ and other native features and primitives is required. Consider using a polyfill for IE < 11 and older browsers. If you're using babel include `"babel-polyfill"` somewhere to your codebase.

although, IE < 11 would not be precise if this is the reason.

Edit: Confirmed that adding babel-polyfill fixed issues in IE11 as well, so the documentation should probably be updated to IE <= 11.

As I'd rather not include 87K of wholesale polyfill in my bundle, do you know if there's a list of polyfills actually needed? I assume these are needed by the intersection observer polyfill, and not react-intersection-observer?

Edit2: Maybe even better, is there a transpiled version of the polyfill as a package around?

@Rendez
Copy link
Contributor

Rendez commented Oct 14, 2017

You're right, it should say to support IE in general, and any other browser lacking support for basic ES2015 features. I'll correct it. That said, you need to polyfill Map/Set/Symbol and their corresponding iterators to work within for...of for this repo, not for the intersection-polyfill.

Normally you would include the babel-polyfill or pick the ones you need by hand within an app. There is also core-js. I encourage you to take a look at babel-preset-env to instead specify which browser support you need in babel, which will selectively do it for you. Our commons babel repo/plugin uses babel-preset-env 😉

In the past I decided to make the source ES2015-compatible only, so I don't include polyfills for ES5 with it. Not everyone needs support for IE, so it's up to you to include the necessary polyfill(s) in the way you see best fit.

Ultimately, you could also build the transpiled package with the polyfill yourself. The lib/js folder is available both from the npm release (npm install-ing the package) and running yarn build manually.

@testower
Copy link
Author

testower commented Oct 14, 2017

Thanks for that. I tried to do that actually, and it didn't really make a difference for IE11.

Just to be clear, is it your module that requires additional polyfilling or is it the intersection-observer polyfill? I would be a bit surprised that a polyfill would depend on other polyfills to be honest.

Here's how I tried to use the transpiled version of your module with a webpack alias:

'react-intersection-observer': '@researchgate/react-intersection-observer/lib/js/index.js'

Would you recommend some other way?

Edit2: That should really not be necessary though, as webpack should automatically select the "main" to import. I'm really not sure what I'm doing wrong here. Including babel-polyfill fixes the issue in IE11, but from what I can tell from your comments, it shouldn't be needed since I'm using the transpiled version?

@Rendez
Copy link
Contributor

Rendez commented Oct 14, 2017

Just to be clear, is it your module that requires additional polyfilling or is it the intersection-observer polyfill?

I decided to make the source ES2015-compatible only, so I don't include polyfills for ES5 with it.

That means that @researchgate/react-intersection-observer requires Map/Set/Symbol to be polyfilled.

I'm really not sure what I'm doing wrong here. Including babel-polyfill fixes the issue in IE11

Indeed, you've fixed the issue. You can select only certain parts of babel-polyfill to leave KBs out. There is more than 1 way to only use the parts of babel-polyfill you need (Map/Set/Symbol in your case), and I described already some of them (babel-preset-env). I don't want to repeat myself as I already gave you the answer, the rest you need to seek out for yourself as it is totally outside the scope of this library.

@testower
Copy link
Author

Please forgive me for making you repeat yourself. I see now I missed your edit on the comment I replied to. There were essentially two things I was asking (which module required polyfilling and exactly which polyfill was required). You have answered both. Thanks for your patience.

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

No branches or pull requests

3 participants