Fixing error with assigning to undeclared variable sourceIsEvt #23
Conversation
Signed-off-by: Charles Hu <[email protected]>
| @@ -30,7 +30,7 @@ var _jsonix_factory = function (_jsonix_xmldom, _jsonix_xmlhttprequest, _jsonix_ | |||
|
|
|||
| // REWORK | |||
| // Node.js | |||
| sourceIsEvt = typeof window !== 'undefined' && window !== null && typeof window.Event === "function" && source instanceof window.Event; | |||
| var sourceIsEvt = typeof window !== 'undefined' && window !== null && typeof window.Event === "function" && source instanceof window.Event; | |||
There was a problem hiding this comment.
What error occurred that prompted you to make this fix? What are the implications of declaring this variable? (Keep in mind that 'use "strict"' does not seem to be set here, so is the introduction of scope as opposed to it automatically being global causing an issue?)
Why did this commit in the weird other js dir do basically the opposite of what you're trying to do now? 8de7f1c
This change was also explicitly made in this commit in this same file: e6e58e6#diff-b77f70d3beb1864de2c1fb848006899f2f2a87b33e72c43980065cef33db3564
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Undeclared_var
There was a problem hiding this comment.
Strict mode is detecting a reference to an undeclared variable and is throwing this (it references the same error as the MDN page you linked):
I'm not sure why the opposite was performed in the original repo, but I see no reason why restricting its scope is detrimental given that it's only ever used in that one instance (albeit I don't know the implications of why it even is global to begin with).
There was a problem hiding this comment.
Though it is true that the original author of the library explicitly removed the var declaration on this line of code. Other forks have added this back:
- highsource@88e56a2
- highsource@46fea9e#diff-b77f70d3beb1864de2c1fb848006899f2f2a87b33e72c43980065cef33db3564
- highsource@e7a42e5
There are 7 forks having activity in the last two years (not counting ours). Of those, 5 forks have commits beyond the original. Of those 5 forks, 4 have updated this line to include the declaration.
All this to say, I do not see an issue with adding the declaration.
It would require a release to update .
| @@ -30,7 +30,7 @@ var _jsonix_factory = function (_jsonix_xmldom, _jsonix_xmlhttprequest, _jsonix_ | |||
|
|
|||
| // REWORK | |||
| // Node.js | |||
| sourceIsEvt = typeof window !== 'undefined' && window !== null && typeof window.Event === "function" && source instanceof window.Event; | |||
| var sourceIsEvt = typeof window !== 'undefined' && window !== null && typeof window.Event === "function" && source instanceof window.Event; | |||
There was a problem hiding this comment.
Though it is true that the original author of the library explicitly removed the var declaration on this line of code. Other forks have added this back:
- highsource@88e56a2
- highsource@46fea9e#diff-b77f70d3beb1864de2c1fb848006899f2f2a87b33e72c43980065cef33db3564
- highsource@e7a42e5
There are 7 forks having activity in the last two years (not counting ours). Of those, 5 forks have commits beyond the original. Of those 5 forks, 4 have updated this line to include the declaration.
All this to say, I do not see an issue with adding the declaration.
It would require a release to update .
sourceIsEvtbeing undeclared before use in script.