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

Make ERB, Handlebars, PHP and Smarty highlight properly in NodeJS #1367

Merged
merged 1 commit into from
Mar 25, 2018

Conversation

Golmote
Copy link
Contributor

@Golmote Golmote commented Mar 23, 2018

Since ERB, Handlebars, PHP and Smarty currently rely on before-highlight and after-highlight hooks to be highlighted properly, they can't be easily highlighted using directly the Prism.highlight() function.

#1357 added the after-tokenize hook to post-process tokens directly in the Prism.highlight() function. This PR adds a before-tokenize hook, and essentially translates what we were previously doing with string replacement, but using the array of tokens.

This should remove the need for the backupCode/before-insert hack, since the code is properly tokenized at the end of the highlight() function.

I also converted the tests from .js format to .test format, since the test runner can now run them normally.

@mAAdhaTTah
Copy link
Member

Is walkTokens similar enough that we can make that reusable in some way between the languages?

@Golmote
Copy link
Contributor Author

Golmote commented Mar 23, 2018

I guess it could, at least for ERB, Handlebars and Smarty (PHP is slightly different). But where would we put it? I'm not really in favor of adding it directly to prism-core...

@paladox
Copy link
Contributor

paladox commented Mar 23, 2018

Testing this on https://codepen.io/anon/pen/dmzpKg it dosen't seem to work.

@mAAdhaTTah
Copy link
Member

I was thinking the Prism.util namespace, but if you don't think it belongs there, it's fine, especially if it's not universally reusable (cuz PHP is different).

@Golmote
Copy link
Contributor Author

Golmote commented Mar 24, 2018

Testing this on https://codepen.io/anon/pen/dmzpKg it dosen't seem to work.

Ruby needs to be included before ERB. See https://codepen.io/anon/pen/eMEgNo

I was thinking the Prism.util namespace, but if you don't think it belongs there, it's fine, especially if it's not universally reusable (cuz PHP is different).

I think this is too much code, used by too few languages (3 or 4 out of 141), to belong in prism-core. But I will keep thinking about this, cause I agree there's far too much repetition here. 😕

@mAAdhaTTah
Copy link
Member

Possibly-terrible-idea: Have all of them depend on a "language" who's sole responsibility is to make this function available on Prism.util.

@Golmote
Copy link
Contributor Author

Golmote commented Mar 24, 2018

Maybe this can be something like prism-templating.js since it's their common point: we need this extra replacement process because those are templating languages, that can be embedded everywhere inside Markup.

I'll give it a try this weekend, we'll see how this goes.

(On a side note, Twig and Django components should probably be rewritten in the same fashion, since they belong to the same kind of languages. Although it can be done at anytime later)

@Golmote
Copy link
Contributor Author

Golmote commented Mar 24, 2018

Ok, so I gave this a try and it works really well.

I managed to make all four of the languages behave the same way. Their definition does not extend Markup anymore. Instead, like PHP does, they switch the grammar to Markup in before-tokenize and use their own grammar in after-tokenize. I guess this could even be a step towards a more generic templating behaviour (not focused only on Markup).

Sorry the diff is getting a bit hard to read.

Copy link
Member

@mAAdhaTTah mAAdhaTTah left a comment

Choose a reason for hiding this comment

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

I like this approach, this makes the language implementations a lot cleaner.

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.

3 participants