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.

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

Fix for use strict #38

Closed
wants to merge 1 commit into from
Closed

Fix for use strict #38

wants to merge 1 commit into from

Conversation

nicholi
Copy link

@nicholi nicholi commented Sep 28, 2017

Inside the final else of the loader section of Hashmap is an innocuous error if 'use strict' is ever added.

	} else {
		// Browser globals (this is window)
		this.HashMap = factory();
	}

With 'use strict' the IIFE actually has no this variable because it is not called via bind/apply. Clearly this was meant as the final fallback to load Hashmap into the global namespace (window) when neither requirejs or amd are used. In a non-strict environment it would do just that, this === window, but with strict we don't get that luxury.

The code to discover "global" is from: https://github.com/purposeindustries/window-or-global/blob/master/lib/index.js
Which technically finds either window/self (browser) or global (node).

I only noticed this problem after running hashmap through a minifier, wherein it also added 'use struct'. I left my fix as optional as possible, so your code flow is unchanged unless this becomes falsey.

I know you aren't using 'use struct' in the project, but it would make it simpler for others if they concat/minify and 'use struct' ever gets added in.

@flesler
Copy link
Owner

flesler commented Sep 28, 2017

Thank you, I'll take a better look at this asap

@flesler
Copy link
Owner

flesler commented Sep 29, 2017

TBH I understand the issue but not a big fan of the solution. It seems super verbose and confusing for an issue that, after many years is the first time that is reported. I don't want to belittle your PR, but I always try to make sure the module is better after each PR and not worse (be it the legibility, maintainability, etc).

Also, I'm running jshint which should emulate use strict and it's not complaining about it. How can I reproduce the error/warning?

@nicholi
Copy link
Author

nicholi commented Sep 29, 2017

If you just toss a 'use strict'; at the top of hashmap.js and attempt to load it into any browser you'll see it halt immediately. Here is a very simple jsfiddle sample showing the same.

jshint never was fully es6 compliant (and strict is not optional in es6, it was in es5), which might be why it's not detecting this scenario.

I understand your trepidation, what if I made the PR a lot simpler? I wasn't sure how many other intended environments you were expecting that final else clause to run under so I went the extra paranoid route. We can make it infinitely simpler, as I only see that condition executing in a browser environment.

@nicholi
Copy link
Author

nicholi commented Sep 29, 2017

Please check the diffs now. We can safely pass 'use strict' now, and the library loads the same in pretty much any environment.

@flesler
Copy link
Owner

flesler commented Sep 29, 2017

Much better now hehe! Two requests:

  1. Rename window to sth that is not a pre-existing prop like scope.
  2. Squash the commits into one with the correct commit message

Thanks

@nicholi
Copy link
Author

nicholi commented Sep 29, 2017

I believe you can control whether to squash the commits when merging on your end? Otherwise I need to open a new PR, correct?

@nicholi
Copy link
Author

nicholi commented Sep 29, 2017

Ok, rebased.

@nicholi nicholi reopened this Sep 29, 2017
@nicholi
Copy link
Author

nicholi commented Oct 2, 2017

Had to make another change. My simpler version of just passing this as parameter into the IIFE wasn't really strict compliant either (for ES6). It will likely just get replaced with void 0, which is useless.

Need to explicitly pass in window/self. The tests just assure that it is in fact an object and has a reference to itself (a common rule for window). This will also make sure when loading via node, you don't get an undefined reference exception.

@nicholi nicholi closed this Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants