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

transpiler: Refactor the transpiler #24

Closed
vicb opened this issue Sep 29, 2014 · 5 comments · Fixed by #36
Closed

transpiler: Refactor the transpiler #24

vicb opened this issue Sep 29, 2014 · 5 comments · Fixed by #36
Assignees

Comments

@vicb
Copy link
Contributor

vicb commented Sep 29, 2014

From #18 (which has been closed):

Some related question: today there is only one dart_class_transformer.js that has all the transformations.

To be more compliant with the original Traceur code, it would probably makes sense to have a DartTransformer that would be the main entry point and every single transformation defined in a separate class & file. All those files should be hosted in a codegeneration folder.

I'm not sure if GH is the best place to discuss this kind of thing. Should we create a "js2dart" room in flowdock ?

This could easily allow running some of the existing transformers which are required. As an example, hoisting is not handled today while a transformer (HoistVariableTransformer exists and can be used)

@naomiblack naomiblack changed the title Refactor the transpiler transpiler: Refactor the transpiler Sep 29, 2014
@vojtajina
Copy link
Contributor

There is a lot of refactoring that needs to be done. What you are saying makes totally sense.

  • separate transformers into classes
  • create two compiler classes (es6+A to dart, es6+A to es5)

and more...

@vicb vicb self-assigned this Sep 30, 2014
@vicb
Copy link
Contributor Author

vicb commented Sep 30, 2014

I have self-assigned this ticket.
Not sure to get what you mean about the two compiler classes. Currently there is a single class that can generate both ES5 and Dart according to the outputLanguage option, what would be the purpose of having 2 different classes ?

@vicb vicb added the ready label Sep 30, 2014
vicb added a commit to vicb/angular that referenced this issue Sep 30, 2014
fix angular#24

The new architecture conforms with the Traceur architecture.
vicb added a commit to vicb/angular that referenced this issue Sep 30, 2014
fixes angular#24

The new architecture conforms with the Traceur architecture.
@vicb vicb closed this as completed in #36 Sep 30, 2014
@vicb vicb removed the ready label Sep 30, 2014
@vicb
Copy link
Contributor Author

vicb commented Sep 30, 2014

@vojtajina I have closed this issue by pushing the refactoring fix to master (I think it's great to have this in master now to build upon it). You should probably create a new issue is more refactoring is needed. Thanks.

@tbosch
Copy link
Contributor

tbosch commented Oct 1, 2014

+1 for keeping only one Compiler that does different things based on
outputLanguage, as traceur already does this for outputLanguage es5 and es6
(the latter only removes annotations and types)...

On Tuesday, September 30, 2014, Victor Berchet notifications@github.com
wrote:

@voltajina I have closed this issue by pushing the refactoring fix to
master (I think it's great to have this in master now to build upon it).
You should probably create a new issue is more refactoring is needed.
Thanks.


Reply to this email directly or view it on GitHub
#24 (comment).

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants