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

"Uncaught ReferenceError: globalThis is not defined" on Android #1299

Closed
mariuskurgonas opened this issue Jan 20, 2023 · 22 comments
Closed

"Uncaught ReferenceError: globalThis is not defined" on Android #1299

mariuskurgonas opened this issue Jan 20, 2023 · 22 comments

Comments

@mariuskurgonas
Copy link

Hello,

Steps to reproduce:

  1. Create a new clean Android project lets say use a single activity template
  2. Add a simple index.html file and latest OSMD minimized library to the assets directory of the project (ofcourse create the assets folder as it does not exist by default)
  3. Make index.html load the library and have a div with id as per wiki tutorial. Also initialize the OSMD and load a simple music xml file.
  4. On the main activity screen, add a webview, call functions on it like enableJavaScript etc.
  5. Load the HTML page from the assets using something like: file:///android_asset/OSMD/index.html
  6. Run the app on simulator

The outcome is - on any simulator or device that runs at least API 29 (Android version 10+) - music sheet shows up without any problems

The issue:

When running the app on say Android 9 (API 28) - music scheet does not show up and console gives the following error:

com.example.myapplication I/chromium: [INFO:CONSOLE(2)] "Uncaught ReferenceError: globalThis is not defined", source: file:///android_asset/OSMD/opensheetmusicdisplay.min.js (2)

It seems that the Android 9 and lower does not support concept of globalThis yet. As i am not a specialized web developer - is there any way to overcome this for cases where globalThis is not found by default?

Thanks you

Marius

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

@mariuskurgonas Can you try this build?
opensheetmusicdisplay.min.js_globalThis_fix.zip

I added null checks on the two occurences of globalThis we use in OSMD's code.
If the problem remains, we'll have to think about whether we can avoid globalThis used by other parts of the build process, like maybe by webpack.

(For reference, the usage is in EngravingRules.setPreferredSkyBottomLineBackendAutomatically() (line 754))

@mariuskurgonas
Copy link
Author

@sschmidTU thanks for a super quick reply

I have added this build to the project but unfortunately its exactly the same problem

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

Are you sure the build wasn't cached?
Can you find out the exact call or line of code that causes the error?
You can find source maps for the build in our Releases.

@mariuskurgonas
Copy link
Author

Yes, i just checked on couple of different versions of Android API 27 and API 26 so on API 27 it gives the same error:

com.example.myapplication I/chromium: [INFO:CONSOLE(2)] "Uncaught ReferenceError: globalThis is not defined", source: file:///android_asset/OSMD/opensheetmusicdisplay.min.fix.js (2)

On API 26 gives slightly different one:

com.example.myapplication I/chromium: [INFO:CONSOLE(2)] "Uncaught SyntaxError: Unexpected token ...", source: file:///android_asset/OSMD/opensheetmusicdisplay.min.fix.js (2)

The min.fix.js ending means that it is using the build from above that you sent.

How do i use that source map? Do i just place it inside the same location as the js file and that will automatically give me more details?

Thanks for the help
Marius

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

Ok, weird, it looks like the old Android doesn't support some basic JS functionalities.

There is only one other use of globalThis, and it also doesn't seem to depend on globalThis being defined.
Here's an unminified build, should give you the exact problem line:
opensheetmusicdisplay_globalThis_fix_unminified.zip

The other use of globalThis except the one I null-checked is by webpack:

/******/ 	(() => {
/******/ 		__webpack_require__.g = (function() {
/******/ 			if (typeof globalThis === 'object') return globalThis;
/******/ 			try {
/******/ 				return this || new Function('return this')();
/******/ 			} catch (e) {
/******/ 				if (typeof window === 'object') return window;
/******/ 			}
/******/ 		})();
/******/ 	})();

I don't see how this can fail if globalThis isn't defined, but maybe you can replace the whole if and try catch blocks with return window.

Maybe there is a compatibility option for webpack that one can set to get a compatible build, or maybe you can research this issue with webpack, see if people found a solution?

@mariuskurgonas
Copy link
Author

The crash seems to be on the line that you updated which is 3575 in the latest file you sent:

const vendor = (_b = (_a = globalThis === null || globalThis === void 0 ? void 0 : globalThis.navigator) === null || _a === void 0 ? void 0 : _a.vendor) !== null && _b !== void 0 ? _b : "";

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

That's once again weird ;) looks like webpack creates some fancy code that prevents the null check.

I added an explicit if, can you check this build now?
opensheetmusicdisplay_globalThis_fix_2_if.zip

It looks like this now in the build:

        if (globalThis) {
            vendor = (_b = (_a = globalThis.navigator) === null || _a === void 0 ? void 0 : _a.vendor) !== null && _b !== void 0 ? _b : "";
            userAgent = (_d = (_c = globalThis === null || globalThis === void 0 ? void 0 : globalThis.navigator) === null || _c === void 0 ? void 0 : _c.userAgent) !== null && _d !== void 0 ? _d : "";
        }

If that build works, try the minified version too:
opensheetmusicdisplay_fix2.min.js.zip

@mariuskurgonas
Copy link
Author

Now it crashes at a different line 34772:

const staffLines = this.musicSystems
.map((musicSystem) => musicSystem.StaffLines)
.flat();

Error is:

"Uncaught (in promise) TypeError: this.musicSystems.map(...).flat is not a function"

As the function flat on the javascript array was introduced in the ES2019, it seems that Android API 28 and older does not support ES19.

I am guessing you are building with this or later version of Javascript updates.

As Android API 28 was released in 2018, the obvious solution would be for you to opt in for something like ES17 but i understand that you might have to rewrite substantial part of code to account for functions like flat etc that were introduced in between.

I wonder is there any other solution that i could apply locally to enable those functions myself like some file that exports all the functions that are missing like flat for array etc. and include them at the top of the lib

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

I added a flat() implementation/polyfill to our Array overloading, can you try the build once more?

opensheetmusicdisplay_fix3.zip

opensheetmusicdisplay_fix3.min.js.zip

We already add a few methods to the prototype in CollectionUtil.ts (that are set to be able to be overwritten by other libraries), so this would be an okay addition.

Maybe we can try eliminating the 3 or so uses of flat() in our code, or try not overwriting the prototype by just using it from our own class.

from CollectionUtil.ts:

if (!Array.prototype.flat) {
    Object.defineProperty(Array.prototype, "flat", {
        enumerable: false,
        writable: true,
        value: function<T>(): any {
            const flatten: any = array => {
                return array.reduce(
                    (flattened, elem) =>
                        flattened.concat(Array.isArray(elem) ? flatten(elem) : elem),
                    []
                );
            };
            return flatten(this);
        }
    });
}

code is from here:
https://gist.github.com/jjant/37fb29edec20f52240915b3816147cfa
(maybe there is a simpler or faster implementation)

sschmidTU pushed a commit that referenced this issue Jan 20, 2023
…is, define Array.flat() method if undefined (#1299)
@mariuskurgonas
Copy link
Author

The interesting part is that in tsconfig.ts the version is set to ES2017, so i am not sure how are you able to build the project if you use the .flat() calls in files.

I just downloaded the whole repo and it gives me warnings that to use flat i need to set target in tsconfig to es2019 :}

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

That's true, I originally wanted to mention that, it just didn't seem to be the cause of the issue.
Interesting, I don't get those warnings.
The flat() usage is from a pull request by an external contributor. We should definitely consider removing the usage.
We could simply add the implementation as a static method (e.g. in CollectionUtil.ts).

@sschmidTU
Copy link
Contributor

@mariuskurgonas Have you tried the new build above? I hoped this would resolve the flat() issue in the build.

@mariuskurgonas
Copy link
Author

Yes, although you have written it in typescript, i deleted the types and its working! It only works on the Android 9 though, as ES2017 is still newer than Android 8 and previous versions, but i think that is good enough for me :)

It would be great if you removed the flat functions as it clearly is in disagreement of the ES2017 target set in your project. I could probably do a pull request but for me it will take some time to figure out as i am not a web developer per se :)

Only other thing is removing the "globalThis" but that i can just delete from code on each new integration as that occurs only 2x through lib so not such a big deal

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

Yes, you're right, one should either stick to the target JS version, or change the target version.
I'll add this as OSMD's own static function instead of using Array.flat() so we're complying with ES2017.

I've tested a few flat() implementations for performance:
https://jsbench.me/ould54ma4z/2
flat3 seems to be one of the fastest, and also the most elegant. (results vary a bit for each run)

const flat3 = arr => {
	return [].concat(...arr);
}
const array = [[1,2],[3,4],[5,6][7,8],[1,2],[3,4],[5,6][7,8],[1,2],[3,4],[5,6][7,8],[1,2],[3,4],[5,6][7,8],[1,2],[3,4],[5,6][7,8]];

The spread operator is from ES6 (2015), so this should be fine.

sschmidTU pushed a commit that referenced this issue Jan 20, 2023
…) implementation to keep ES2017 target from tsconfig.json (#1299)
@mariuskurgonas
Copy link
Author

Sounds great. I also found some other stuff that is from ES2018 like {...var, ...var2} type of syntax has 104 uses through the library where ES2017 syntax would be Object.assign(obj1, obj2);

Probably someone should go over ES2018 and ES2019 standards and search for those patterns through the project. I might do it some time if no one else does it and create a pull request.

Here is a good list of ES standards, what was introduced to each version and all browsers compatibilities:

https://kangax.github.io/compat-table/es2016plus/

I will fiddle around more with even older Android versions and till try to find all the uses of ES2018 stuff and might either report about it or do a pull request "fixing" it. It would mean that it would probably start working on Android version 8 so it would be even better for me :}}}

Thanks for all your help in regards of this matter still wish that you could resolve that globalThis as well but so be it if you can not do that easily as it is a tiny fraction compared to other things that we discussed here.

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

@mariuskurgonas New build, hopefully this finally fixes this ;) could you test it again?
opensheetmusicdisplay_fix4.zip

opensheetmusicdisplay_fix4.min.js.zip

Code see here:
#1301


I also found some other stuff that is from ES2018 like {...var, ...var2} type of syntax has 104 uses through the library where ES2017 syntax would be Object.assign(obj1, obj2);

Are you sure about that? The spread operator (...) is ES6 / ES2015 (source).

So is {...var, ...var2} a special syntax that's not covered by the spread operator from E2015?
If it's indeed not covered by E2017, where did you find that syntax in OSMD?

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 20, 2023

Only other thing is removing the "globalThis" but that i can just delete from code on each new integration as that occurs only 2x through lib so not such a big deal

still wish that you could resolve that globalThis as well but so be it if you can not do that easily as it is a tiny fraction compared to other things that we discussed here.

@mariuskurgonas Isn't the globalThis problem resolved by my commits and build above? globalThis being undefined should not cause build issues anymore.
There might be a way to hide the globalThis check behind another if bracket, maybe by checking whether window.globalThis exists or something.

@mariuskurgonas
Copy link
Author

@sschmidTU

The globalThis still crashed the app as it could not be found, but i noticed that webpack has a safe way to checking if globalThis exist or not and this is the solution that worked:

if (typeof globalThis === 'object') { // it looks like globalThis can be undefined and cause build issues in es2017 (e.g. Android API 28), see #1299 vendor = (_b = (_a = globalThis.navigator) === null || _a === void 0 ? void 0 : _a.vendor) !== null && _b !== void 0 ? _b : ""; userAgent = (_d = (_c = globalThis.navigator) === null || _c === void 0 ? void 0 : _c.userAgent) !== null && _d !== void 0 ? _d : ""; }

After i changed the if check - it all worked fine, flat fix also worked immediately.

Now talking about the spread operator, es2018 instroduced something different called Rest Spread

Here is the original proposal:

https://github.com/tc39/proposal-object-rest-spread

While you are completely right that spread operator itself was introduced like in ES2015 or smth, this time its called rest spread and example in your library would be at line 44211:

const options = { ...this.options, ...this.piece.options };
Before this was introduced, ES2017 used this syntax:

Object.assign(obj1, obj2);
Or an example with a resulting object

const one_big_object = Object.assign({}, obj1, obj2, obj3, etc);

So in your case the line mentioned above should change to

const options = Object.assign(this.options, this.piece.options);

When i mentioned that there are 104 cases like this through the library i may have been wrong as i have only searched for ... which in some cases is ok from ES2015.

@mariuskurgonas
Copy link
Author

Now it crashes at a different line 34772:

const staffLines = this.musicSystems .map((musicSystem) => musicSystem.StaffLines) .flat();

Error is:

"Uncaught (in promise) TypeError: this.musicSystems.map(...).flat is not a function"

As the function flat on the javascript array was introduced in the ES2019, it seems that Android API 28 and older does not support ES19.

I am guessing you are building with this or later version of Javascript updates.

As Android API 28 was released in 2018, the obvious solution would be for you to opt in for something like ES17 but i understand that you might have to rewrite substantial part of code to account for functions like flat etc that were introduced in between.

I wonder is there any other solution that i could apply locally to enable those functions myself like some file that exports all the functions that are missing like flat for array etc. and include them at the top of the lib

I realized that when i replied this, i had removed globalThis in general instead of using your if fix, so by accident i made you believe that your if function worked... Sorry about that

@sschmidTU
Copy link
Contributor

sschmidTU commented Jan 21, 2023

While you are completely right that spread operator itself was introduced like in ES2015 or smth, this time its called rest spread and example in your library would be at line 44211:

const options = { ...this.options, ...this.piece.options }; Before this was introduced, ES2017 used this syntax:

This seems to be done by webpack, because I can't find any occurence of {... or { ... in our code.
Actually, Vexflow itself has one occurence of this. We do have a VexFlowPatch system where we could patch this out, though one has to wonder how long we can keep this up to eliminate this rest spread that apparently es2018 introduced, also when we upgrade to Vexflow 4+, which likes to use the latest typescript features.
Does this actually also not work in Android API 28?
We can definitely add this one patch for the currently used Vexflow version (1.2.93) temporarily.

The globalThis still crashed the app as it could not be found, but i noticed that webpack has a safe way to checking if globalThis exist or not and this is the solution that worked:

if (typeof globalThis === 'object')

@mariuskurgonas I added this check to the build on the PR branch, can you try it out once more? :)
opensheetmusicdisplay_fix5.zip

opensheetmusicdisplay_fix5.min.js.zip

@mariuskurgonas
Copy link
Author

@sschmidTU thanks for everything

Not sure where the rest spread comes from but in the final build there are several instances of it that i could see from a quick search.

I guess nevermind about that because from my current tests the library works as far as on Android API 27 which is Android version 8.0 and upwards. Theoretically speaking if you could take care of that spread bit it might work on API 26, but you can not have pie and eat it as they say, your explanation seems too big of a work to bother.

I have dropped in the minimized build you attached and it worked on Android API 27 upwards so all seems good thank you

I think you can close the issue now if you are to include these changes for flat and globalThis in the next release

Again thank you for a very quick solution really appreciate it!!

Best regards,
Marius

@sschmidTU
Copy link
Contributor

@mariuskurgonas Haha, true, can't have your cake and eat it too. At some point older JS and Android support will have to be dropped, even if just for dependencies like Vexflow.
Happy the current build works, that'll be in the next release 1.7.3 then, very soon!
Thank you for your cooperation as well, handy to have this tested on various Android versions!
Kind regards

sschmidTU added a commit that referenced this issue Jan 21, 2023
…ray.flat() from ES2019 (#1299, PR #1301)

* fix es2017 incompatibility (e.g. android api 28): null-check globalThis, define Array.flat() method if undefined (#1299)

* comment

* remove Array.flat() usage (ES2019 addition), add CollectionUtil.flat() implementation to keep ES2017 target from tsconfig.json (#1299)

* replace last .flat() usage (forgot to commit)

* try another globalThis undefined/typeof check for es2017 compatibility (#1299)

Co-authored-by: sschmidTU <s.schmid@phonicscore.com>
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

No branches or pull requests

2 participants