Skip to content

Add stubs for missing downlevel feature transforms, organize transforms by purpose #1270

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

Merged
merged 2 commits into from
Jun 24, 2025

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jun 23, 2025

Making this its' own PR (since it's a pure refactor) aughta make reviewing the followups with each feature transform implementation much easier, as each transform has its' own associated baseline diff changes.

With this change, the root transformers package only contains the definition of a root Transformer object and utilities used by multiple classes of transforms. In practice, this doesn't change much for consumers, since only a single function in the emitter ever touches the actual transform factory functions and is the only place package names really needed to change. The organization is just for maintenance purposes, since the count of files in the folder was starting to get unwieldy with individual es transforms (and the count of those is guaranteed to increase as time goes on).

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the transformer codebase by grouping downlevel feature transforms into purpose-specific subpackages, updating factory/visitor method calls, and adjusting transformer registration.

  • Moved various helper functions and utilities into tstransforms, moduletransforms, estransforms, and jsxtransforms packages
  • Renamed unexported factory and visitor fields to exported Factory() and Visitor() calls across all transform code
  • Updated emitter setup in internal/compiler/emitter.go to use the new subpackage-based transformer factories

Reviewed Changes

Copilot reviewed 37 out of 38 changed files in this pull request and generated no comments.

File Description
internal/transformers/utilities.go Exported and reduced root utilities; relocated many helpers
internal/transformers/tstransforms/*.go Introduced TS-specific transforms and moved helpers
internal/compiler/emitter.go Rewired script/module transformers to use new subpackages
Comments suppressed due to low confidence (2)

internal/transformers/estransforms/forawait.go:17

  • The function name newforawaitTransformer does not follow CamelCase conventions. Consider renaming to newForAwaitTransformer for clarity and consistency.
func newforawaitTransformer(emitContext *printer.EmitContext) *transformers.Transformer {

internal/compiler/emitter.go:57

  • The logic in getScriptTransformers is very similar to the implementation in internal/transformers/transformer.go. Consider centralizing this transformer-registration logic to avoid duplication and simplify future maintenance.
func getModuleTransformer(emitContext *printer.EmitContext, options *core.CompilerOptions, resolver binder.ReferenceResolver, getEmitModuleFormatOfFile func(file ast.HasFileName) core.ModuleKind) *transformers.Transformer {

@jakebailey
Copy link
Member

With the multiple functions, is the intent that we are doing multiple passes for the same ES version now?

@weswigham
Copy link
Member Author

With the multiple functions, is the intent that we are doing multiple passes for the same ES version now?

Yeah, by keeping them per-feature, we can minimize what each feature transform walks. Since we have the flags space now, we can also swap the subtree flags from ContainsESXXXX to the actual transformed feature in question (though some years only have one feature anyway). And, going forward, feature transforms like this are easier to migrate from esnext to a fixed year number when they stablize.

@jakebailey
Copy link
Member

I thought the whole point of keeping them together was for perf? Or am I misunderstanding what this PR will do?

RE: the flags, we still have to interop with JS, so I don't think we actually end up with more space, right?

@weswigham
Copy link
Member Author

RE: the flags, we still have to interop with JS, so I don't think we actually end up with more space, right?

We do need more space in practice - in strada we use 30/31 bits on transform flags already, which is why every time we add a new ESXXX transform we scrounge around and swap some marker bits to lazy caluclation (and I don't think these are exposed on the API presently...? Or if they are, we really aughta think about a more-than-31-bit-flag world for it). In corsa we're currently only using 19 bits, though, because a lot of the marker bits were for es3/5 features, so using some extra bits to break down the transforms some is fine.

Basically, we've compromised for API compat in the past, and I'd like to stop, while we're breaking strict API compatibility anyway~

@jakebailey
Copy link
Member

Back compat is fine to break sure, I just mean compat with JS only having 32 bits to work with. But, it sounds like we're going to have more space from dropping ES3/5 anyway.

@weswigham
Copy link
Member Author

But, it sounds like we're going to have more space from dropping ES3/5 anyway.

Yeah, that is the case, but I also doubt subtree facts are gonna be exposed on the API this time anyway - transform flags were always an implementation optimization that required specific, correct node factory usage to be setup, and the replacement we have in corsa is the same (and is arguably more cumbersome to use correctly outside the compiler's transform pipeline happy path). I don't know why we'd want to expose the "subtree contains await" bit flag or similar flags to JS API users when we don't have to.

@jakebailey
Copy link
Member

I'd think it'd matter for roundtripping reasons for doing AST transforms across the JS barrier

@weswigham
Copy link
Member Author

weswigham commented Jun 23, 2025

In that case, you don't need these flags to make a node, as they're derived from the AST shape as-constructed, so the JS side doesn't need them to roundtrip in the serialized version of a node. In fact, they shouldn't roundtrip so they can be recalculated based on the mutated AST - that's what I meant by "not easy to use outside the transform pipeline happy path".

@weswigham weswigham added this pull request to the merge queue Jun 24, 2025
Merged via the queue into microsoft:main with commit 1e34fb9 Jun 24, 2025
22 checks passed
@weswigham weswigham deleted the es-dl branch June 24, 2025 00:21
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.

3 participants