Skip to content

Use esModuleInterop flag, adopt star-imports - #214144

Merged
Johannes Rieken (jrieken) merged 8 commits into
mainfrom
joh/intense-heron
Jun 4, 2024
Merged

Use esModuleInterop flag, adopt star-imports#214144
Johannes Rieken (jrieken) merged 8 commits into
mainfrom
joh/intense-heron

Conversation

@jrieken

Copy link
Copy Markdown
Contributor

No description provided.

@@ -77,11 +77,20 @@ export function connectProxyResolver(
}

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

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

@pankajk07 Pankaj Khandelwal (pankajk07) Jun 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Johannes Rieken (@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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Johannes Rieken (@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.

Copy link
Copy Markdown
Contributor

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