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

entities are being replaced with characters #48

Closed
GavinJoyce opened this issue Apr 26, 2019 · 15 comments · Fixed by #50
Closed

entities are being replaced with characters #48

GavinJoyce opened this issue Apr 26, 2019 · 15 comments · Fixed by #50

Comments

@GavinJoyce
Copy link
Collaborator

Input of &lt; &gt; &times; is resulting in output of < > ×

related issue: glimmerjs/glimmer-vm#833

@rwjblue
Copy link
Member

rwjblue commented Apr 26, 2019

FWIW, this is a large issue over in prettier's side too (changing &nbsp; to ) due to the underlying issue. I'd personally be happy to add a mode to the glimmer parser to avoid translation of entities (also happy to point folks in the right direction on how to wire that up) that we could use for thigns like this lib, prettier, ember-template-recast, etc where you want a source -> source transformation instead of normal precompilation.

@tylerturdenpants
Copy link
Collaborator

Point away. I'm interested.

@GavinJoyce
Copy link
Collaborator Author

@rwjblue I'd be interested in these pointers too. Having worked with HBS codemods a little, it's clear that there is an opportunity to make some fundamental changes which will enable much more comprehensive tooling possible. @patocallaghan and I are keen on helping get tools like prettier for hbs into a great place and addressing these fundamental issue seems like a good first step.

@rwjblue
Copy link
Member

rwjblue commented Apr 30, 2019

Sorry :( I did a first pass here: glimmerjs/glimmer-vm#938

@rwjblue
Copy link
Member

rwjblue commented Apr 30, 2019

This PR fixes the entity issue (nicely), and makes it simpler to deal with whitespace after handlebars block opening/closing.

@tylerturdenpants
Copy link
Collaborator

@rwjblue will a release be cut soon?

@rwjblue
Copy link
Member

rwjblue commented Apr 30, 2019

Needs review (I pinged folks), but shouldn't take too long to land + release once its been reviewed.

@GavinJoyce
Copy link
Collaborator Author

Nice one!

@rwjblue
Copy link
Member

rwjblue commented Apr 30, 2019

We should setup a time to brainstorm on the general problem though. The fix I submitted will only help some of the issues, and we will continue to have to fix things in convoluted ways due to the Handlebars parser being so lossy. Things like {{~ preservation are a good example of the fundamental failure scenarios.

I think we will ultimately need to do a custom parser (we already differ from Handlebars in enough ways that I don't think this is terrible) that has better semantics around preserving enough info in the AST to be able to print properly. This would help basically everyone in the space (prettier / glimmer / ember / template-lint / codemods / etc could all use it)...

@tylerturdenpants
Copy link
Collaborator

@GavinJoyce whoever has the free time to take this up should take it up. I can create releases, so let me know when I can help.

@GavinJoyce
Copy link
Collaborator Author

We should setup a time to brainstorm on the general problem though

👍 That would be great. I'll ping you on discord to set something up

@GavinJoyce
Copy link
Collaborator Author

From #dev-ember-js on discord:

Screen Shot 2019-04-30 at 21 40 21

@Alonski
Copy link
Contributor

Alonski commented May 17, 2019

Just pinging here to ask how this is moving along :)

@rwjblue
Copy link
Member

rwjblue commented May 17, 2019

I published the update to @glimmer/syntax to allow opting in to "codemod" mode as of 0.41.0, if we update to that (and tweak the options we pass) we should be able to completely avoid issues with these HTML entity characters.

@patocallaghan
Copy link
Collaborator

I've a PR to update @glimmer/syntax here https://github.com/rajasegar/ember-angle-brackets-codemod/pull/50

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 a pull request may close this issue.

5 participants