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.

Sign up for GitHub

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

Fixing error with assigning to undeclared variable sourceIsEvt #23

Merged
merged 1 commit into from
Aug 23, 2023
Merged

Fixing error with assigning to undeclared variable sourceIsEvt #23

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nodejs/scripts/jsonix.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

@Amndeep7 Amndeep7 Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Screenshot 2023-08-23 at 8 18 56 AM

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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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 .


if (!sourceIsEvt && source.hasOwnProperty && source.hasOwnProperty('toString')) {
destination.toString = source.toString;
Expand Down