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

refactor(meta_generator): use template string #3696

Merged
merged 1 commit into from
Aug 31, 2019

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Aug 31, 2019

What does it do?

Only a minor changes, use template string instead of replace().

How to test

git clone -b meta-generator https://github.com/sukkaw/hexo.git
cd hexo
npm install
npm test

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@SukkaW SukkaW requested a review from curbengh August 31, 2019 09:08
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.225% when pulling e0cba1f on SukkaW:meta-generator into f57f700 on hexojs:master.

@curbengh curbengh merged commit eca3cf2 into hexojs:master Aug 31, 2019
@curbengh
Copy link
Contributor

curbengh commented Aug 31, 2019

btw, I just found an edge case for this helper. <title> is also used in <svg>, so it can issue when inline embed svg.

A more robust approach is to only append to </title> inside <head></head>, since <svg> shouldn't be inside there.

@SukkaW
Copy link
Member Author

SukkaW commented Aug 31, 2019

@curbengh

/<head>.*?<title>.*?(<\/title>).*?<\/head>/

I think this pattern will do the job. I will open another PR.


Update

@curbengh
replace() with substr will only replace first occurrence. The edge case is about someone put a svg before <title> in <head>, which will only happened when someone inline a svg in <style> and put it before <title> like this:

<html>
<head>
<style>
  .foo {
    background: url(data:image/svg+xml;utf8,<svg width="500" height="300" xmlns="http://www.w3.org/2000/svg"><g><title>svg</title><rect x="10" y="10" width="200" height="50" style="fill:none; stroke:blue; stroke-width:1px"/></g></svg>);
  }    
</style>
<title>TITLE</title>
</head>
...
</html>

I think it might happened.

@SukkaW
Copy link
Member Author

SukkaW commented Aug 31, 2019

/<head>(?!<\/head>).+?<\/head>/

If we have to back to replace </head>, we might use this pattern to avoid <head></head> issue.

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