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 don't take source_type param #6248

Closed
overlookmotel opened this issue Oct 2, 2024 · 2 comments · Fixed by #6251
Closed

Transformer don't take source_type param #6248

overlookmotel opened this issue Oct 2, 2024 · 2 comments · Fixed by #6251
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Comments

@overlookmotel
Copy link
Contributor

Currently Transformer::new takes a source_type param:

pub fn new(
allocator: &'a Allocator,
source_path: &Path,
source_type: SourceType,
source_text: &'a str,
trivias: Trivias,
options: TransformOptions,
) -> Self {
let ctx = TransformCtx::new(source_path, source_type, source_text, trivias, &options);
Self { ctx, options, allocator }
}

Can we get rid of it? source_type is present in Program and parser updates it from "unambiguous" to either "script" or "module".

It seems to make more sense to use the value of source_type present in Program, than allow user to over-ride it.

I can't see any circumstance in which that's a useful thing to do, and it's a potential footgun. It's confusing that in transformer it's possible that self.ctx.source_type.is_script() != !self.ctx.source_type.is_module(), because it can also be "unambiguous". That's not theoretical - I hit this problem while trying to pass some TypeScript tests.

If for some obscure reason, the user does want to override it, they can always alter the value of Program::source_type before passing Program to transformer.

@overlookmotel overlookmotel added the C-bug Category - Bug label Oct 2, 2024
@Boshen
Copy link
Member

Boshen commented Oct 2, 2024

Yes.

I also wanted a bigger API change for Source with path, text and related friends. But you may make this smaller breaking API change first.

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Oct 2, 2024

I also wanted a bigger API change for Source with path, text and related friends.

Ah ha! Yes, that'd be nice. Maybe we could also combine it with oxc-project/backlog#94?

let source = Source::from_file(path);

This would initialize a Source with the provided path, SourceType as deduced from the path's file extension, and also read the file from file system (using fast SIMD UTF-8 validation). A one-stop shop!

(we'd also provide more complex methods to create Source from a String, override SourceType etc)

But you may make this smaller breaking API change first.

OK great. I'll do that now.

@overlookmotel overlookmotel added C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior A-transformer Area - Transformer / Transpiler and removed C-bug Category - Bug labels Oct 2, 2024
@Boshen Boshen closed this as completed Oct 3, 2024
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-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants