pgn-viewer icon indicating copy to clipboard operation
pgn-viewer copied to clipboard

'use strict' in dist bundle vs undefined

Open gcp opened this issue 5 years ago • 7 comments

I'm adapting https://github.com/gcp/rchesspgn to use this PGN viewer.

One issue that I hit is that the amalgamated .js file in the dist/ dir defines 'use strict', but then tries to use variables that are undefined. This will break parsing at that point, at least when trying to incorporate it as a WebExtension.

undefined access if the config var isn't set by the user: https://github.com/mliebelt/PgnViewerJS/blob/master/dist/js/pgnvjs.js#L10508

This is supposed to fall through if module is not defined, but that causes undefined access: https://github.com/mliebelt/PgnViewerJS/blob/master/dist/js/pgnvjs.js#L3724

gcp avatar Dec 24 '19 14:12 gcp

I guess the second one is because in a WebExtension context, the following should be set on this instead of on window: https://github.com/mliebelt/PgnViewerJS/blob/master/dist/js/pgnvjs.js#L10526

gcp avatar Dec 27 '19 12:12 gcp

I am not Javascript-savvy enough to understand your point here. What should I do to ensure that all variables are defined? What are the variables in my code that are undefined? Do you have references for pattern how to address this? I tried to find a reference on WebExtensions, but could not find something to understand the problem you have to solve.

So when I know what to do, I can try to do that. Have you tried to clone my application, and fix this problem on a branch? This would allow to me to understand the problem better ...

mliebelt avatar Jan 03 '20 23:01 mliebelt

if (window.PgnBaseDefaults.localPath) {

Checks whether window.PgnBaseDefaults has a localPath property. But in fact, if window.PgnBaseDefaults is undefined, we try to look up a property on an undefined object. JavaScript is typically very lax and will continue (I think actually creating the property), but the source code contains use strict definitions, which turn this into an error: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

gcp avatar Jan 04 '20 19:01 gcp

The second problem is much more obscure. The code assumes the global object is the window object and sets the i18n config on that, but at least when being executed as a WebExtension (i.e. a Firefox, Chrome or Edge add-on) content script this doesn't appear to be the case.

I will try to get some more info here and advise if replacing window by this is a proper fix.

gcp avatar Jan 04 '20 19:01 gcp

I found this post https://stackoverflow.com/questions/44671610/sandboxed-this-in-firefox-webextension-content-script but it doesn't have a satisfactory answer. Both objects are set in a content script so this shouldn't be an XRay vision problem.

gcp avatar Jan 06 '20 15:01 gcp

Filed the second issue as: https://bugzilla.mozilla.org/show_bug.cgi?id=1607242

We'll see if this behavior is intentional.

gcp avatar Jan 06 '20 16:01 gcp

I wish I had properly filed 2 issues now. Anyway, my suggestion here: https://github.com/mliebelt/PgnViewerJS/issues/128#issuecomment-569261183 to replace window by this in that line of code appears to be correct. The code should then work both as a webpage and as an add-on.

https://bugzilla.mozilla.org/show_bug.cgi?id=1208775#c3 https://bugzilla.mozilla.org/show_bug.cgi?id=1208775#c5

The problem is that in Firefox, in an add-on, this !== window, yet the current code expects exactly that.

gcp avatar Jan 06 '20 17:01 gcp