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

transformer: research inserting helper methods with import or require #7396

Open
Boshen opened this issue Nov 21, 2024 · 8 comments
Open

transformer: research inserting helper methods with import or require #7396

Boshen opened this issue Nov 21, 2024 · 8 comments
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Comments

@Boshen
Copy link
Member

Boshen commented Nov 21, 2024

cjs vs esm 😡

@Boshen Boshen added C-bug Category - Bug A-transformer Area - Transformer / Transpiler labels Nov 21, 2024
@Boshen Boshen self-assigned this Nov 21, 2024
@overlookmotel
Copy link
Contributor

Helper methods should already inserted as import statements / require depending on source_type. What's missing?

@Boshen Boshen added this to the Transformer Milestone 2 milestone Nov 25, 2024
@Boshen
Copy link
Member Author

Boshen commented Nov 25, 2024

@Boshen
Copy link
Member Author

Boshen commented Nov 25, 2024

There is one failing test case in monitor-oxc due to this confusion:

file:///home/runner/work/monitor-oxc/monitor-oxc/node_modules/.pnpm/@[email protected][email protected]/node_modules/@azure/core-rest-pipeline/dist/esm/policies/proxyPolicy.js:2
import { HttpProxyAgent } from "http-proxy-agent";
         ^^^^^^^^^^^^^^
SyntaxError: The requested module 'http-proxy-agent' does not provide an export named 'HttpProxyAgent'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:171:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:254:5)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5)

We inserted the wrong import in one of the files in http-proxy-agent

@overlookmotel
Copy link
Contributor

I'm confused. Our transforms only insert imports from @babel/runtime and react. We didn't insert that import.

It's present in the original source:

proxy

https://www.npmjs.com/package/@azure/core-rest-pipeline?activeTab=code

@Boshen
Copy link
Member Author

Boshen commented Nov 25, 2024

The transformer transformed files in http-proxy-agent, which breaks node.js because it cannot detect esm anymore.

It has nothing to do with the importer file.

@Boshen
Copy link
Member Author

Boshen commented Nov 27, 2024

Actual offending file: https://cdn.jsdelivr.net/npm/[email protected]/dist/index.js

I need to figure out why it's set as a module

/// Insert `import` / `require` statements at top of program.
fn insert_into_program(&self, transform_ctx: &TransformCtx<'a>, ctx: &mut TraverseCtx<'a>) {
if transform_ctx.source_type.is_script() {
self.insert_require_statements(transform_ctx, ctx);
} else {
self.insert_import_statements(transform_ctx, ctx);
}
}

@Boshen
Copy link
Member Author

Boshen commented Nov 27, 2024

"js" | "mjs" | "jsx" => (Language::JavaScript, ModuleKind::Module),

If we want to make this proper, we need to start .js as Unambiguous ... that'll be a lot of things to fix 🤔

@overlookmotel
Copy link
Contributor

I still don't get what's going on in that failing test with http-proxy-agent. So maybe I'm missing the point again, but...

In monitor-oxc, shouldn't we be determining whether a file is ESM or CommonJS by looking for a type field package.json? This code is running in NodeJS, so there should be no ambiguity, and no need to search the code for import / export statements to make a guess.

In the case of http-proxy-agent there is no type field present in package.json, so it should be treated as CommonJS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug
Projects
None yet
Development

No branches or pull requests

2 participants