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

Add a Converter back to Javascript #154

Closed
wants to merge 10 commits into from

Conversation

jogibear9988
Copy link
Contributor

No description provided.

@jogibear9988
Copy link
Contributor Author

see also: jquery/esprima#2076

@jogibear9988 jogibear9988 marked this pull request as draft April 28, 2021 22:46
@ejsmith
Copy link

ejsmith commented May 10, 2021

This would be amazing! Are you basing this code on escodegen? https://github.com/estools/escodegen/blob/master/escodegen.js

@jogibear9988
Copy link
Contributor Author

no, but maybe that would be a good idea...
at the moment I've no time to work on this, but I think I'll come back in a month or two and then I'll take a look to base it on escodegen.

@lahma
Copy link
Collaborator

lahma commented May 11, 2021

Esprima has the same JSON presentation as JS counterpart so I would think that parsing the program, creating JSON from it, feeding it to Jint as object and then evaluating the escodegen.js against it should produce the JS in question, at least in theory.

@jogibear9988
Copy link
Contributor Author

jogibear9988 commented May 11, 2021

Yes, but I don't think I would do that. That would be much slower (at runtime) than converting escodegen to a c# classand using it.
Doesn't seem to be to big, so I think when I've time that will not be such a huge task

@jogibear9988
Copy link
Contributor Author

@sebastienros would you think that should be a class in esprima-dotnet or should i create an extra github repo for it?

@sebastienros sebastienros changed the base branch from dev to main June 14, 2021 22:00
@sebastienros
Copy link
Owner

I think being able to write the javascript back is indeed a good thing, and quite often requested.
Could be by porting the escodegen project, or creating a custom visitor like you did. Ideally it should take formatting settings so we can either be lossless from the parsed tree, or apply custom formatting settings like space management.

@jogibear9988
Copy link
Contributor Author

i'll work on this one again when the rest is merged 😊
for my project I'll need this, but I need ES6 class support.
I've a webserver in kestrel, and i will use it ad a live minifier...

@jogibear9988
Copy link
Contributor Author

@lahma @sebastienros
So I've now a workin Version. With this converter my complex WebApp works, wich uses over 400 Javascript Files.
Many big libraries are used (like Polymer, Esprima, Jquery, ... and everything seems to work).

At the Moment this converter adds much to many brackets to the code. This could be optimized over time.

@sebastienros
Copy link
Owner

I assume you deserve some sleep now.

@jogibear9988
Copy link
Contributor Author

;-) it's only half past 1 in germany

@jogibear9988
Copy link
Contributor Author

@sebastienros @lahma
so this is a first workin version. it add's much too many brackets, but it works. this could be optimized then.
I still need to fix coding style, but maybe you can look if the code is okay like this.

I also think the visitor pattern is done wrong, we don't return the expressions, so I could not use the visitor to modify the tree.
When you see here: https://github.com/microsoft/referencesource/blob/master/System.Core/Microsoft/Scripting/Ast/ExpressionVisitor.cs every visit method returns the expression. So also the AstVisitor in esprima-net is not like this.

@jogibear9988
Copy link
Contributor Author

I've merged my bigint pull here to, but I will rebase once it's merged. I need it cause or code uses BigInt

@jogibear9988
Copy link
Contributor Author

I think there also will be packages were the converter still will fail, but I've tested a few wich are in use in our application:

jquery, jquery-ui, monaco editor, polymer3, fontawesome, cropperjs, dock-spwan-ts

so most javascript should work atm.

@jogibear9988 jogibear9988 mentioned this pull request Nov 3, 2021
@jogibear9988
Copy link
Contributor Author

@sebastienros @lahma
what do you think, does the expressionVisitor & AstVisitor need to be changed, so they can also modify a tree?

@jogibear9988
Copy link
Contributor Author

@sebastienros @lahma I've now fixed the coding to match esprima-net style, so I think this is ready for review

Copy link
Owner

@sebastienros sebastienros left a comment

Choose a reason for hiding this comment

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

Can you also fix the build warning?

src/Esprima/Utils/ToJavascriptConverter.cs Outdated Show resolved Hide resolved
src/Esprima/Utils/ToJavascriptConverter.cs Outdated Show resolved Hide resolved
src/Esprima/Utils/ToJavascriptConverter.cs Outdated Show resolved Hide resolved
@jogibear9988
Copy link
Contributor Author

??


namespace Esprima.Utils
{
public static class ToJavascriptConverterExtension
Copy link
Owner

Choose a reason for hiding this comment

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

Make it internal for now and add ToJavaScriptString() on Program.
The idea is that we parse, get an instance of Program (the parse tree) and can call ToJavaScriptString() on it directly, without having to deal with an alternate class.
Making it internal will let us change it later without breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do so, I need to copy the whole class to my Code, cause I use it to minify ES6 Template Literals with HTML & CSS inside, see:

public class BaseCustomWebcomponentMinifier : ToJavascriptConverter
{
    public static string ToJavascript(Node node)
    {
        var visitor = new BaseCustomWebcomponentMinifier();
        visitor.Visit(node);
        return visitor.ToString();
    }

    protected override void VisitPropertyDefinition(PropertyDefinition propertyDefinition)
    {
        if (TryGetParentAt(1) is ClassBody && propertyDefinition.Static && propertyDefinition.Key is Identifier i)
        {
            if (i.Name == "style")
            {
                if (propertyDefinition.Value is TaggedTemplateExpression tte)
                {
                    if (tte.Tag is Identifier iT && iT.Name=="css")
                    {
                        if (tte.Quasi is TemplateLiteral tl)
                        {
                            if (tl.Quasis.Count==1 && tl.Quasis[0] is TemplateElement)
                            {
                                var q = tl.Quasis[0];
                                var css = q.Value.Raw;
                                try
                                {
                                    var parser = new CssParser();
                                    var ss = parser.ParseStyleSheet(css);
                                    var newCss = ss.ToCss(new AngleSharp.Css.MinifyStyleFormatter());
                                    css = newCss;
                                }
                                catch (Exception ex)
                                { }

                                _sb.Append("static style=css`" + css + "`;");
                                return;
                            }
                        }
                    }
                }
            }
            if (i.Name == "template")
            {
                if (propertyDefinition.Value is TaggedTemplateExpression tte)
                {
                    if (tte.Tag is Identifier iT && iT.Name == "html")
                    {
                        if (tte.Quasi is TemplateLiteral tl)
                        {
                            if (tl.Quasis.Count == 1 && tl.Quasis[0] is TemplateElement)
                            {
                                var q = tl.Quasis[0];
                                var html = q.Value.Raw;
                                try
                                {
                                    var parser = new HtmlParser();
                                    var h = parser.ParseDocument(html);
                                    var newHtml = h.ToHtml(new AngleSharp.Html.MinifyMarkupFormatter());
                                    html = newHtml;
                                }
                                catch (Exception ex)
                                { }

                                _sb.Append("static template=html`" + html + "`;");
                                return;
                            }
                        }
                    }
                }
            }
        }
        base.VisitPropertyDefinition(propertyDefinition);
    }
}

cause of that I also wanted to convert the Visitor to a real Visitor wich can return a modified AST

var program = parser.ParseScript();
var code = ToJavascriptConverter.ToJavascript(program);

Assert.Equal("if(true){p();}switch(foo){case 'A':p();break;}switch(foo){default:p();break;}for(var a=[];;){}for(var elem of list){}", code);
Copy link
Owner

Choose a reason for hiding this comment

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

This is the only test that actually checks the rendered javascript. Ideally any javavascript in should be the same out, but if we can't keep the original text because we should at least export with a common indentation pattern, and be able to check the output is fine. Maybe a better test in this case is to take any javascript file (call it source1), parse it, render it (call it source2) and ensure that parsing and rendering source2 is the same as source2.

This can be done for all subsequent tests in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can fix the tests...
I many used them to test parts wich did not work, I will create an really expected output.
Of cause we need indention on the converter (if beautification is enabled), but I had no time atm to work on this.

Copy link
Owner

Choose a reason for hiding this comment

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

Is it generating valid JavaScript?

_sb.Append("static style=css`" + css + "`;");

If so, could you update the parse tree before it's rendered to JS instead?
If not then we'll have to live with this class being public for now. Or you could do some InternalVisibleFrom trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes
i could not update the tree, cause everthing is readonly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cause of that I wanted a real visitor pattern like the ExpressionVisitor in C#, wich could return modified Expressions

@sebastienros
Copy link
Owner

I can do the changes I requested if you want me to help.

@jogibear9988
Copy link
Contributor Author

@sebastienros I did now a simple beautification.
And fixed most of the tests with an assert

@jogibear9988
Copy link
Contributor Author

@sebastienros hello, sorry to be impatient, but any more issues with this or could it be merged?

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Left some comments. Great initiative and if it's not yet perfect, we need some dogfooding anyway. Would be a great candidate for some v3 beta release.

@@ -67,6 +66,30 @@ public static AssignmentOperator ParseAssignmentOperator(string op)
};
}

public static string ConvertAssignmentOperator(AssignmentOperator op)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should probably be internal

@@ -84,6 +84,39 @@ public static BinaryOperator ParseBinaryOperator(string op)
};
}

public static string ConvertBinaryOperator(BinaryOperator op)
Copy link
Collaborator

Choose a reason for hiding this comment

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

internal

@@ -50,6 +50,23 @@ private static UnaryOperator ParseUnaryOperator(string? op)
};
}

public static string ConvertUnaryOperator(UnaryOperator op)
Copy link
Collaborator

Choose a reason for hiding this comment

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

internal

}
}

public class ToJavascriptConverter
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this implementation details, like internal sealed, would name like JavascriptWriter or something like so make more sense?

@lahma
Copy link
Collaborator

lahma commented Jun 8, 2022

Maybe we should resurrect this one, and schedule for v3? This needs a rebase and maybe a peer review from @adams85 (if you have time for it).

@jogibear9988
Copy link
Contributor Author

When the ast refactoring is merged, I'll look to rebase this

@adams85
Copy link
Collaborator

adams85 commented Jun 9, 2022

Sure, I'll take a look at it after rebase has been done.

@jogibear9988
Copy link
Contributor Author

i've rebase this branch in #291

@jogibear9988 jogibear9988 mentioned this pull request Jul 4, 2022
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.

None yet

5 participants