amoilanen / sniffr

Browser, os and device detection
MIT License
42 stars 5 forks source link

Defining the exports object in the global environment breaks the libs on the client side #14

Closed DErekXx closed 7 months ago

DErekXx commented 9 months ago

Hi.

The latest update in the library brings new lines of code:

if (typeof window !== 'undefined') { window.exports = window.exports || {} }

It's breaking the libraries these are defining the environment and putting itself on the object's dependence on the environment

var globalObject = typeof window === 'object' && window || typeof self === 'object' && self; if(typeof exports !== 'undefined') { factory(exports); } else if(globalObject) { globalObject.hljs = factory({}); }

In this case, all of the client libraries that do not bundling or importers as modules are defending in the exports global object, which is not expected. Because these should be defined in the global window object.

amoilanen commented 7 months ago

Thanks for reporting, going to investigate further.

window.exports = window.exports || {} should not override an existing "exports" property but I see from the snippet posted that it might result in some other libraries thinking that there is a real "exports" property defined on the window object => break these libraries

Going to think of an alternative solution of dealing with window.exports and how to avoid this situation, going to publish an update to the library once the solution is clear.

DErekXx commented 7 months ago

Thanks for the answer. I just want to put here an example of what I mentioned above. So, we unfortunately still use the old version of the library of the higliht.js, which was broken after updating your library. Here is the particular line of the code https://github.com/Nalis/highlight.js/blob/cf4b46e5b7acfe2626a07914e1d0d4ef269aed4a/src/highlight.js#L14

  var globalObject = typeof window === 'object' && window ||
                     typeof self === 'object' && self;

  if(typeof exports !== 'undefined') {
    factory(exports);
  } else if(globalObject) {
    globalObject.hljs = factory({});
  }

That led to another way to initialize the library, in the main usage case we init the library from the global object.

We solved it temporarily, just overriding the previous version inside the packages.json.

  "overrides": {
      "sniffr": "1.2.1"
  },

And unfortunately for us, it's no one example of the old-style library, that brings big refactoring inside legacy code.

Thanks for your help and passion.

amoilanen commented 7 months ago

@DErekXx The regression should be fixed in 4e056fea0045d9134679113bae7eec81afa87aec

I removed the extra lines from if (typeof window !== 'undefined') { window.exports = window.exports || {} } from the NPM package version

Also in the standalone version the exports variable should no longer be accidentally leaked to the global scope.

The updated version 1.3.2 with the fix has been published to npmjs.com

Please, let me know if the issue has been fully resolved/further changes might be needed and reopen this ticket if needed