Fixes#16541
Hi,
this is an attempt to fix#16541 , feedback is greatly appreciated!
I was not able to test if the result works as expected, I just adjusted
the code and manually created a test snapshot from the mocha console
output until I thought it looked right.
Could someone tell me...
1. how to actually build the `@pulumi/pulumi` package and how to test
the fixes quickly?
2. how to create a snapshot for a new closure test?
3. if there is a quick way to write the TypeScript code in this project,
set breakpoints and run tests against it?
- I plan to investigate the pnpm bundling bug of the `CallbackFunction`
but working on this and testing the code is extremely tedious right now
as I do not know your secrets (yet 😀)
- I didn't find these infos in the `CONTRIBUTING.md` and other READMEs
- I am very familiar with TypeScript and the nodejs tooling, but no clue
how everything is stitched together in this repo
- feel free to send me a Slack message in the Pulumi Community or
suggesting another communication channel
Thanks!
fyi @julienp
This commit addresses part of #11942, in which we fail to serialise
closures whose code would use reserved identifiers like `exports`. This
is due to a change we made where module imports are hoisted to avoid
importing the same module multiple times. Previously, code adopted a
strategy of passing all dependencies using a `with` statement, viz.:
```typescript
function x() {
return (function () {
with({ fooBarBaz: require("foo/bar/baz"), ... }) {
// Use of fooBarBaz
}
}).apply(...)
}
function y() {
return (function () {
with({ fooBarBaz: require("foo/bar/baz"), ... }) {
// Use of fooBarBaz
}
}).apply(...)
}
```
This was changed to remove the duplicate imports, yielding code like:
```typescript
const fooBarBaz = require("foo/bar/baz")
function x() {
return (function () {
with({ ... }) {
// Use of fooBarBaz
}
}).apply(...)
}
function y() {
return (function () {
with({ ... }) {
// Use of fooBarBaz
}
}).apply(...)
}
```
However, while the previous approach would work with reserved
identifiers such as `exports` (`with({ exports, ... }) { ... }` is
perfectly acceptable), the new one does not (`const exports = ...` is
not acceptable since NodeJS will not allow redeclaration of the
`exports` global).
This commit combines the two approaches. Modules are only imported once,
but if an import would use a reserved identifier, we generate a fresh
non-conflicting identifier and alias this using a `with` statement. For
example:
```typescript
const fooBarBaz = require("foo/bar/baz")
// __pulumi_closure_import_exports is generated to avoid shadowing the reserved "exports"
const __pulumi_closure_import_exports = require("some/other/module")
function x() {
return (function () {
with({ exports: __pulumi_closure_import_exports, ... }) {
// Use of fooBarBaz and exports
}
}).apply(...)
}
```
Note that it is not expected that #11942 will be solved in its entirety.
While this commit fixes code that introduces identifiers like `exports`,
the introduction in question in that issue is caused by the use of
`pulumi.output(...)` in the constructor of a dynamic resource provider.
Since dynamic resource providers are implemented under the hood by
serialising their code, we attempt to serialise `pulumi.output` and its
dependency chain. This commit allows us to make more progress in that
regard, but other things go wrong thereafter. Fixing the deeper issue
that underpins #11942 (and likely other challenges with dynamic
providers) is probably a more involved piece of work.
<!---
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->
# Description
When encountering a function obtained via `Function.bind` we previously
failed to serialise the function because we could not get the function
text.
This PR uses the v8 debugger API to grab the internal
`[[TargetFunction]]` property to obtain the original function and
re-bind it.
This does currently not handle successive binds like
```(function f() { ... }).bind("a").bind("b")```
Ref https://github.com/pulumi/pulumi/issues/5294
## Checklist
- [ ] I have run `make tidy` to update any new dependencies
- [ ] I have run `make lint` to verify my code passes the lint check
- [ ] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Cloud,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in the service repo. -->
# Description
Emit expressions for aliased native functions
https://github.com/pulumi/pulumi/issues/5294#issuecomment-2031404191
## Checklist
- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
- [x] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [x] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
<!---
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->
# Description
Update the list of node modules detected as builtin during function
serialisation.
This helps with some of the cases in
https://github.com/pulumi/pulumi/issues/5294 (notably trying to use
`fs/promises`).
## Checklist
- [ ] I have run `make tidy` to update any new dependencies
- [ ] I have run `make lint` to verify my code passes the lint check
- [ ] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [ ] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
# Description
Builds on top of https://github.com/pulumi/pulumi/pull/15753
Fixes https://github.com/pulumi/pulumi/issues/15735
There are a couple breaking changes in the typescript API that we use in
`sdk/nodejs/runtime/closure/rewriteSuper.ts`. This PR adds a shim that
is used to bridge the differences and versions the snapshots where
needed.
This does not make typescript a peer dependency yet. Instead the tests
force a specific version to be used via [yarn
resolutions](https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/).
## Checklist
- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
- [x] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [x] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
# Description
In preparation of https://github.com/pulumi/pulumi/issues/15735 we make
the closure tests proper integration tests so that we can run them with
different typescript versions. Move each test to its own folder instead
of one large file.
This PR only changes tests, and does not touch any of the function
serialisation code.
Some snapshots had to be updated for indentation changes.
Hidden after all the test cases is the test script
[sdk/nodejs/tests/runtime/testdata/closure-tests/test.ts](dfcc953c08/sdk/nodejs/tests/runtime/testdata/closure-tests/test.ts)
Verified that tests run in CI
https://github.com/pulumi/pulumi/actions/runs/8389170587/job/22975068167?pr=15753
## Checklist
- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
- [x] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [x] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->