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

Use esModuleInterop flag, adopt star-imports #214144

Merged
merged 8 commits into from
Jun 4, 2024
Merged

Use esModuleInterop flag, adopt star-imports #214144

merged 8 commits into from
Jun 4, 2024

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Jun 3, 2024

No description provided.

@jrieken jrieken self-assigned this Jun 3, 2024
@jrieken jrieken enabled auto-merge (squash) June 3, 2024 14:52
@vscodenpa vscodenpa added this to the June 2024 milestone Jun 3, 2024
@jrieken jrieken disabled auto-merge June 3, 2024 14:57
@jrieken jrieken requested a review from bpasero June 3, 2024 14:57
bpasero
bpasero previously approved these changes Jun 3, 2024
@jrieken jrieken enabled auto-merge June 4, 2024 09:52
@jrieken jrieken merged commit d02c5b0 into main Jun 4, 2024
39 checks passed
@jrieken jrieken deleted the joh/intense-heron branch June 4, 2024 10:22
@@ -77,11 +77,20 @@ export function connectProxyResolver(
}

function createPatchedModules(params: ProxyAgentParams, resolveProxy: ReturnType<typeof createProxyResolver>) {

function proxyAssign(module: any, patch: any) {
Copy link

@pankajk07 pankajk07 Jun 7, 2024

Choose a reason for hiding this comment

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

@jrieken Any reason for not adding a setter for this proxy?

Due to this change, our Postman extension is failing in the latest Insider Release, as we need to re-assign tls.createSecureContext for https://github.com/postmanlabs/postman-request/blob/00acf088467643fafb5b5c5bb0100f05a6ec5cca/index.js#L185

Issue reported to us: postmanlabs/postman-app-support#12923

VS Code Issue raised at #214584

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrieken Any reason for not adding a setter for this proxy?

No special reason, we don't need it and therefore didn't add it. I can investigate doing this

as we need to re-assign tls.createSecureContext for

FYI that long term, once ESM is actually happening, this won't be an option anymore. So, please be on the lookout for an alternative. Already today, the star-import wrapper that TS generates doesn't allow module modification anymore. Hence, the proxy trick.

Choose a reason for hiding this comment

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

No special reason, we don't need it and therefore didn't add it. I can investigate doing this

Thanks. Please have a look at it and provide a setter so that it doesn't break existing extensions

FYI that long term, once ESM is actually happening, this won't be an option anymore. So, please be on the lookout for an alternative. Already today, the star-import wrapper that TS generates doesn't allow module modification anymore. Hence, the proxy trick.

Can you shed some more light on it?

I tried to create a minimal implementation by reassigning the module function, and it seems to work Code Sandbox

@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants