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: do not always overwrite global.performance #2922

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

robertkowalski
Copy link
Contributor

@robertkowalski robertkowalski commented Jun 8, 2023

new node version already have a performance global, see https://nodejs.org/api/globals.html#performance

it contains many methods, see https://nodejs.org/api/perf_hooks.html

when always overwriting it, other modules like undici which use the performance object won't work any more. in genera using the performance object become unusable as soon as the @cdktf/hcl2json package is required.

discussion/bug: renovatebot/renovate#22615

Related issue

Fixes #

Description

In plain English, describe your approach to addressing the issue linked above. For example, if you made a particular design decision, let us know why you chose this path instead of another solution.

Checklist

  • I have updated the PR title to match CDKTF's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 8, 2023

CLA assistant check
All committers have signed the CLA.

@robertkowalski
Copy link
Contributor Author

I forked the repository to run all the unit tests

@robertkowalski
Copy link
Contributor Author

Screenshot 2023-06-08 at 13 33 29

Copy link
Member

@mutahhir mutahhir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution!!

As mentioned in the header of this file (wasm_bridge_exec), we tend to follow Golang's wasm_exec_node. Can you update the PR to match that (specifically this)?

@robertkowalski
Copy link
Contributor Author

@mutahhir updated and adjusted the commit message

@robertkowalski
Copy link
Contributor Author

@mutahhir with the changes to crypto unit tests fail, i will revert the changes for crypto

$ node ./lib/__tests__/edge-provider-schema/cli.js ./edge-provider-bindings
/__w/terraform-cdk/terraform-cdk/packages/@cdktf/hcl2json/wasm/wasm_exec.js:288
						crypto.getRandomValues(loadSlice(sp + 8));
						       ^

TypeError: crypto.getRandomValues is not a function

new node version already have a performance global, see
https://nodejs.org/api/globals.html#performance

it contains many methods, see https://nodejs.org/api/perf_hooks.html

when always overwriting it, other modules like `undici` which use
the performance object won't work any more. in general using the
performance object becomes unusable as soon as the `@cdktf/hcl2json`
package is required.

discussion/bug: renovatebot/renovate#22615

this PR adjusts the checked in sourcefile to the latest upstream version:
https://github.com/golang/go/blob/ee46f0b5084461978432aa20df003ac52500b0f0/misc/wasm/wasm_exec_node.js#L12-L17
Copy link
Member

@mutahhir mutahhir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you! 🏅

@mutahhir mutahhir added this pull request to the merge queue Jun 16, 2023
Merged via the queue into hashicorp:main with commit dbf0467 Jun 16, 2023
@mutahhir mutahhir mentioned this pull request Jul 5, 2023
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants