Use typeof BigInt to check for BigInt support

[lukasIO]

fixes #486

[CLAassistant]

All committers have signed the CLA.

[smaye81]

Thanks for the PR @lukasIO. It's definitely unfortunate that globalThis is only supported in ES2020 and above.

It looks like we have a few other uses of globalThis, however. For example, take a look at packages/protoplugin/src/ecmascript/generated-file.ts. Would you be willing to fix those occurrences also and put up a few unit tests to ensure we're fixing all the right things?

[lukasIO]

Sure, I can do that!

Should the unit tests go into the protobuf-test package?
How do you usually run that (didn't see any scripts that invoke jest)?

[smaye81]

It depends what you are testing. Anything testing functionality in packages/protobuf is tested in packages/protobuf-test. If you are testing protoplugin code, then the tests should go in packages/protoplugin-test.

For running tests, you can run make test from the app root. If you want to run tests for a specific package, you can run:

• make test-protobuf
• make test-protoplugin

Thanks again!

[lukasIO]

put up a few unit tests to ensure we're fixing all the right things

I realised I'm not sure what you're looking for test wise regarding the changes in this PR.
Still happy to write them as part of this PR, but could you elaborate on what specifically you'd like to test for?

[timostamm]

If we refer to Number without the globalThis modifier, it can clash with a protobuf message named "Number". We have to avoid this situation. Our generated code should always compile.

[lukasIO]

would adding Number to the reservedIdentifiers suffice for that situation? (the lowercase number is already in there)

[timostamm]

would adding Number to the reservedIdentifiers suffice for that situation? (the lowercase number is already in there)

It should automatically avoid name clashes by generating a class named Number$ instead of Number for a protobuf message named Number.

But this is technically a breaking change. If any user is using such a protobuf message, we break their generated code, which we definitely want to avoid.

I don't have an immediate idea to solve this 🤔

[lukasIO]

Are you sure it would be a breaking change? Afaict using Number as a protobuf message name would potentially break already today.

If globalThis.Number was emitted until now that means that the environment in which the code was running had a global variable called Number defined already, which means the generated protobuf message would clash with the globally defined Number. Obviously trying to argue for my case here, but that sounds like a bug fix to me 👀

[smaye81]

Hi @lukasIO. Sorry for the delay, but we did some thinking on how best to solve this. To answer your question, it would be a breaking change. If we started generating a class name of Number$ to avoid the name clash, then we would break the code of anyone using Number for a message name.

However, we think we've come up with a way to remove the globalThis usage while still not breaking anyone's code. Take a look at #504, which should solve this problem.

I think as far as this PR is concerned, you can revert your changes to packages/protoplugin/src/ecmascript/generated-file.ts and we can land your change as-is to proto-int64. This PR coupled with #504 should hopefully solve the globalThis issue.

[lukasIO]

nice! reverted the other changes.
Let me know if you prefer typeof BigInt === 'function' over the current typeof BigInt !== 'undefined'

[smaye81]

Let's change it to typeof BigInt === 'function' to stay consistent with the other checks and to be more precise.

[smaye81]

re: the CI failure, you just need to run make locally and push up the changes (looks to just be the code benchmark README).

[lukasIO]

hm, getting an error on make now running locally

protobuf-es/.tmp/protobuf-23.0/src/google/protobuf/BUILD.bazel:749:14: Linking src/google/protobuf/libtest_messages_proto3_proto.so failed: (Exit 1): cc_wrapper.sh failed: error executing command external/local_config_cc/cc_wrapper.sh @bazel-out/darwin-fastbuild/bin/src/google/protobuf/libtest_messages_proto3_proto.so-2.params

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
ld: illegal thread local variable reference to regular symbol __ZN6google8protobuf8internal15ThreadSafeArena13thread_cache_E for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

running make bench by itself does work however. not sure if something else is needed. feel free to push up any other changes that are required.

[smaye81]

Merged. Thanks for the PR @lukasIO!