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

feat(transformer): add transform-typescript boilerplate #2866

Merged

Conversation

Boshen
Copy link
Member

@Boshen Boshen commented Mar 30, 2024

No description provided.

Copy link
Member Author

Boshen commented Mar 30, 2024

@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label Mar 30, 2024
@Boshen Boshen force-pushed the 03-30-feat_transformer_add_transform-typescript_boilerplate branch from 4edbbfe to fb1f63b Compare March 30, 2024 10:23
Copy link

codspeed-hq bot commented Mar 30, 2024

CodSpeed Performance Report

Merging #2866 will not alter performance

Comparing 03-30-feat_transformer_add_transform-typescript_boilerplate (fb1f63b) with main (c1a2958)

Summary

✅ 36 untouched benchmarks

@Boshen
Copy link
Member Author

Boshen commented Mar 30, 2024

A plugin can be turned off, we haven't decided which one is better:

pub struct Transformer<'a> {
    typescript: Option<TypeScript>,
}

or

#[derive(Debug, Default, Clone)]
pub struct TransformOptions {
    pub typescript: Option<TypeScriptOptions>,
}

A None indicates the plugin is turned off, both methods have tradeoffs, we'll bike-shed this later.

@Boshen Boshen requested review from rzvxa and Dunqing March 30, 2024 10:42
@rzvxa
Copy link
Collaborator

rzvxa commented Mar 30, 2024

A plugin can be turned off, we haven't decided which one is better:

pub struct Transformer<'a> {
    typescript: Option<TypeScript>,
}

or

#[derive(Debug, Default, Clone)]
pub struct TransformOptions {
    pub typescript: Option<TypeScriptOptions>,
}

A None indicates the plugin is turned off, both methods have tradeoffs, we'll bike-shed this later.

Aren't they both needed? A None option means the plugin is not turned on and would result in a None transformer.

@Boshen
Copy link
Member Author

Boshen commented Mar 30, 2024

Aren't they both needed? A None option means the plugin is not turned on and would result in a None transformer.

True. I was thinking about eliminating if let Some(plugin) = &self.plugin and hide this check inside each individual plugin.

I'll do some follow up PRs.

@rzvxa
Copy link
Collaborator

rzvxa commented Mar 30, 2024

Well, I guess we can have the plugin options set to default when it's disabled but It means that we need to have some other way to configure our list of plugins. What is your alternative for it? When fetching the options to initialize the transformer we need to return None anyway in order to notify the Transformer to not create this plugin.

@rzvxa
Copy link
Collaborator

rzvxa commented Mar 30, 2024

True. I was thinking about eliminating if let Some(plugin) = &self.plugin and hide this check inside each individual plugin.

You mean when calling the transform methods inside our main Transformer?

@Boshen
Copy link
Member Author

Boshen commented Mar 30, 2024

The previous version had something like this all over the place

  self.es2021_logical_assignment_operators.as_mut().map(|t| t.add_vars_to_statements(stmts));
  self.es2020_nullish_coalescing_operators.as_mut().map(|t| t.add_vars_to_statements(stmts));
  self.es2016_exponentiation_operator.as_mut().map(|t| t.add_vars_to_statements(stmts));
  self.es2015_arrow_functions.as_mut().map(|t| t.transform_statements(stmts));

I was thinking about changing it to

self.es2021_logical_assignment_operators.add_vars_to_statements(stmts);

And hide the Option check inside the add_vars_to_statements function.

This seems like a burden to plugin authors, not a really good idea.

I'll bike-shed this a bit later.

@rzvxa
Copy link
Collaborator

rzvxa commented Mar 30, 2024

Yes, we have to wait to make the call on this one until the mutability is figured out, Since as we discussed in here we might need a macro for applying the transformation which can also be used to hide these checks.

@Boshen Boshen merged commit 293b9f4 into main Mar 30, 2024
33 checks passed
@Boshen Boshen deleted the 03-30-feat_transformer_add_transform-typescript_boilerplate branch March 30, 2024 12:48
Copy link
Member Author

Boshen commented Mar 30, 2024

Merge activity

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

Successfully merging this pull request may close these issues.

3 participants