Skip to content

fix(js): allow impit usage from ESM#74

Merged
barjin merged 2 commits intomasterfrom
fix/exports-esm
Feb 25, 2025
Merged

fix(js): allow impit usage from ESM#74
barjin merged 2 commits intomasterfrom
fix/exports-esm

Conversation

@barjin
Copy link
Member

@barjin barjin commented Feb 25, 2025

Renames native code import to mitigate naming collision with global exports.

@barjin barjin self-assigned this Feb 25, 2025
@barjin barjin merged commit 6954466 into master Feb 25, 2025
9 checks passed
@barjin barjin deleted the fix/exports-esm branch February 25, 2025 13:44
@B4nan
Copy link
Member

B4nan commented Feb 25, 2025

huh, did this really broke usage in ESM projects? and how come things work fine when we were testing it in crawlee? those projects were also ESM

@barjin
Copy link
Member Author

barjin commented Feb 25, 2025

Right - the other way around - this might have broken usage in CJS projects.

I'm assuming this broke the Crawlee E2E tests on Platform, as we run Local and Memory by importing directly from the test.mjs scripts (ESM context), but on Platform, we first build it with tsc (with tsconfig.json extending @apify/tsconfig, so it compiles with module: CommonJS).

@barjin
Copy link
Member Author

barjin commented Feb 25, 2025

see the bad run e.g. here:

https://console.apify.com/view/runs/bic7Rmg3L4sh4yF8n

@B4nan
Copy link
Member

B4nan commented Feb 25, 2025

But impit is CJS, there is no native ESM build, the same code was executed regardless of the caller being CJS or ESM.

I am just confused that this ever worked, regardless of the project using it was CJS/ESM.

@barjin
Copy link
Member Author

barjin commented Feb 25, 2025

Ah, took me a while, but I think I got to the bottom of this. My guess would still be on some transpilation tool adding a shim for exports (better than Node does). Those tools (like ESBuild) are abundant in our projects because of the usage of TypeScript and frameworks like vite, which just make everything work automagically.

See this JS example mixing ESM and CJS syntax - node ./esm.js will fail with the "Identifier 'exports' has already been declared".

obrazek

Now see e.g. this example from esbuild with --format=esm - when transpiled, ESBuild will rename the original exports shim to exports2 to avoid conflict (so the result will be executable).

obrazek

You're still right that it's weird that this would touch the code from dependencies. 🤔

Also, I'm not sure what the takeaway from this is. Maybe that we should add e2e tests to impit? :)

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