Should not set the global variable "scrollMonitor" when in AMD environment

[aripalo]

First, thanks for the awesome library. It does its job very well 👍

---

Just to nitpick, the current implementation defines global variable window.scrollMonitor explicitly in index.js#L10:

if (isInBrowser) {
  window.scrollMonitor = scrollMonitor;
}

This is somewhat unwanted functionality in AMD environments (especially when one is developing cross-site widgets and want to avoid conflicts with globals on host sites).

As a solution, I propose we get rid of the explicit global window.scrollMonitor definition and we let the Webpack Universal Module Definition target template handle it as you already use it:

(function webpackUniversalModuleDefinition(root, factory) {
    if(typeof exports === 'object' && typeof module === 'object')
        module.exports = factory();
    else if(typeof define === 'function' && define.amd)
        define([], factory);
    else {
        var a = factory();
        for(var i in a) (typeof exports === 'object' ? exports : root)[i] = a[i];
    }
})(this, function() {
  // the rest of the webpack bootstrap ...
  // actual scrollMonitor scripts ...

Which basically checks if in:

1. CommonJS environment
2. AMD environment
3. Attach to root/global, which in browserland is window … and then initializes the library for the first environment it detects.

[stutrek]

Good catch! I'm on vacation I'll take a look next week.

On Mon, Mar 20, 2017 at 3:38 PM Ari Palo notifications@github.com wrote:

First, thanks for the awesome library. It does its job very well 👍

Just to nitpick, the current implementation defines global variable window.scrollMonitor explicitly in index.js#L10 https://github.com/stutrek/scrollMonitor/blob/f88afe9b9b066176778169750f66369390588eb6/index.js#L10 :

if (isInBrowser) { window.scrollMonitor = scrollMonitor; }

This is somewhat unwanted functionality in AMD environments (especially when one is developing cross-site widgets and want to avoid conflicts with globals on host sites).

As a solution, I propose we get rid of the explicit global window.scrollMonitor definition and we let the Webpack Universal Module Definition target template handle it as you already use it https://github.com/stutrek/scrollMonitor/blob/v1.2.3/webpack.config.js#L9-L11 :

(function webpackUniversalModuleDefinition(root, factory) { if(typeof exports === 'object' && typeof module === 'object') module.exports = factory(); else if(typeof define === 'function' && define.amd) define([], factory); else { var a = factory(); for(var i in a) (typeof exports === 'object' ? exports : root)[i] = a[i]; } })(this, function() { // the rest of the webpack bootstrap ... // actual scrollMonitor scripts ...

Which basically checks if in:

1. CommonJS environment
2. AMD environment
3. Attach to root/global, which in browserland is window … and then initializes the library for the first environment it detects.

---

You can view, comment on, or merge this pull request online at:

https://github.com/stutrek/scrollMonitor/pull/62 Commit Summary

• Remove explicit window.scrollMonitor definition

File Changes

• M index.js https://github.com/stutrek/scrollMonitor/pull/62/files#diff-0 (4)

Patch Links:

• https://github.com/stutrek/scrollMonitor/pull/62.patch
• https://github.com/stutrek/scrollMonitor/pull/62.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/stutrek/scrollMonitor/pull/62, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUtfzWfTRSBFCOraC2001-bFkHiEeA6ks5rnp2OgaJpZM4MimuH .

[aripalo]

Enjoy your vacation! 🍹

I can live with my fork (but I still think this is something that would be good to fix for others/future).

[stutrek]

I see what you did here. Originally I had it like this, until #19. Initially I didn't implement it because, like you, I thought it was a little weird. Since then I ran into some use case myself (maybe it was testing? I can't remember) and I decided that pragmatism is the way to go: just let people use it like that. Unfortunately, now that it's like this (and I'm not saying it was a good idea), changing it back would be a major version change because someone might be using it... foolishly.

Thanks for the contribution. Keep making the world around you better.

[aripalo]

Totally understandable! 👍 And as I said, I can live with my fork!