andrewplummer / Sugar

A Javascript library for working with native objects.
https://sugarjs.com/
MIT License
4.53k stars 306 forks source link

Sugar throwing a runtime exception in Angular v6 application. #632

Open phixMe opened 6 years ago

phixMe commented 6 years ago

I have used the following import syntax for the Sugar date module through Angular v5.2.10 and CLI v1.7.4 with success. Upon upgrading the version of Angular and its companions I observe the following issue reported by Sugar v2.0.4.

import * as Sugar from 'sugar/date';

image

I have also tried the following without success...

import {Sugar} from 'sugar/date';

I understand that this may be an Angular related issue, but I was curious if others have observed this upon upgrading their Angular applications.

andrewplummer commented 6 years ago

Thanks for this! I'll have a look into it too but for a workaround can you get in there and try to figure out which global it's trying to reference?

phixMe commented 6 years ago

I'll try to help out the best I can without being too familiar with the internals of the library.

It looks like this error occurs during the mapping of the NATIVE_NAMES constant specifically when name === 'Object'

forEachProperty(NATIVE_NAMES.split(' '), function(name) {
  createNamespace(name);
}); 

image

// seems to be undefined
globalContext["Object"].prototype; 
YurkaninRyan commented 6 years ago

When you log globalContext what do you get @phixMe?

I recently was having this problem in firefox content scripts. It looks like globalContext defaults to this, but in firefox this is not the window. This may be related.

tasuku-s commented 6 years ago

I doubt that strict mode makes this object undefined.

I fixed it by putting this workaround into polyfills.ts.

window['global'] = window;
vendethiel commented 6 years ago

I doubt that strict mode makes this object undefined.

That's one of the features of strict mode, though: https://coderwall.com/p/jes4dw/strict-mode-this-keyword-in-javascript

gaiottino commented 6 years ago

We just ran into this issue as well. @tasuku-s does your workaround have any consequences in terms of AOT etc?

tasuku-s commented 6 years ago

@gaiottino I have no idea to answer this question. But, it works with AOT build mode in my application.

I wonder why strict mode makes this undefined only under angular6 environment. It seems like global issue, if sugar.js apply this to globalContext under strict mode. In actual, sugar.js bind this without strict mode. https://github.com/andrewplummer/Sugar/blob/master/dist/sugar.js#L13473

So maybe webpack or typescript wrap around with strict mode. There is some option to solve this issue.

andrewplummer commented 6 years ago

Hi, sorry for the delay on this. I had a look into this issue.

It seems that there are a couple of factors here. First it seems that @tasuku-s is right that there is a wrapper with strict mode going on that makes the assumed this become not the global context. However this seems to be standard webpack behavior. However in addition there's something wonky going on with Angular's webpack environment that doesn't define a global keyword. This causes a fallback to standard browser environment, but of course webpack isn't a standard browser environment, so you are essentially running code in some kind of hybrid environment which is going to make edge cases come up. Standard webpack doesn't display this behavior. I'd be surprised if other libs haven't been hit by this as well.

The fix here is I think for Sugar to stop making assumptions about the environment it's being run in and perform more robust checks for both the global context as well as exports . Essentially, no more this, just get global or window explicitly.

jikamens commented 5 years ago

Is https://github.com/andrewplummer/Sugar/commit/0cd73d1fc83e16a7b51e02c5b19b9db18d7ac180 intended to be the fix for this issue?