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

Remove usage of globalThis in generated code #504

Merged
merged 2 commits into from
Jun 8, 2023
Merged

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Jun 7, 2023

This removes the usage of globalThis in generated code by instead using an export of global Number constants.

This is done so that we can safely use these global constants when generating code and be assured we're using the correct values. We cannot use globalThis since we support ES2017 and globalThis was introduced in ES2020. We also don't want to explicitly generate code using, for example, Number.NaN, since this could clash with a message name of Number.

Instead we can export them in a local proto-double.js file since this will be in a different scope as the generated code and we are guaranteed to use the intended global values.

Note that this complements #488 to remove usage of globalThis in the repo.

@smaye81 smaye81 requested a review from timostamm June 7, 2023 19:01
@smaye81 smaye81 merged commit fb74adc into main Jun 8, 2023
@smaye81 smaye81 deleted the sayers/globalthis branch June 8, 2023 14:32
@smaye81 smaye81 mentioned this pull request Jul 14, 2023
smaye81 added a commit that referenced this pull request Jul 14, 2023
## What's Changed
* Add `toPlainMessage` to convert `Message` objects to their
`PlainMessage` variants by @srikrsna-buf in
#511
* Remove usage of globalThis in generated code by @smaye81 in
#504
* Use `typeof BigInt` to check for BigInt support by @lukasIO in
#488

## New Contributors
* @lukasIO made their first contribution in #488.

**Full Changelog**:
https://github.com/bufbuild/protobuf-es/compare/v1.2.1..v1.3.0
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.

2 participants