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

[Help Wanted] Transition from String to AST parsing. #54

Open
TheLarkInn opened this issue Feb 8, 2017 · 3 comments · May be fixed by #70
Open

[Help Wanted] Transition from String to AST parsing. #54

TheLarkInn opened this issue Feb 8, 2017 · 3 comments · May be fixed by #70

Comments

@TheLarkInn
Copy link
Owner

From the latest few patch releases, it is becoming apparent that we should probably leverage AST parsing over Regex to provide these replacements.

Since I am 200% overbooked myself, I can't work on this myself atm. I would love for someone to take on this task. The existing tests should still be valid for passing, and maybe even a few more could be added.

Thank you everyone for contributing to issues and enhancements.

@Kefaise
Copy link

Kefaise commented Apr 5, 2017

I found other bug with regexp approach. When we have comment after templateUrl, for example
templateUrl: './app.template.html' /*my awesome template*/
it cannot satisfy closing bracket or comma from regexp
var templateUrlRegex = /templateUrl\s*:(\s*['"`](.*?)['"`]\s*([,}]))/gm;
And it is not replaced.

@Riobe
Copy link

Riobe commented Apr 27, 2017

It looks like if we were doing javascript, then it would be relatively straightforward with esprima + escodegen, but I looked and didn't see equivalent tools for typescript.

Considering that every example of this parsing that you're testing for and supporting are in experimental decorators, I'd say a typescript is a reasonable assumption. Do you know of any tools that can take an AST parsing from the typescript language services and output code based on that? Even just the parsing looks much less straightforward...

@Riobe
Copy link

Riobe commented Apr 27, 2017

@Kefaise I believe my pull request #67 will handle your scenario as well. I just added a test case for it as well. I think @TheLarkInn is right that AST would likely be best, but this might still be helpful in the meantime.

tmair pushed a commit to tmair/angular2-template-loader that referenced this issue Jun 24, 2017
@tmair tmair linked a pull request Jun 24, 2017 that will close this issue
tmair pushed a commit to tmair/angular2-template-loader that referenced this issue Jun 24, 2017
tmair pushed a commit to tmair/angular2-template-loader that referenced this issue Jun 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants