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

replaceSkippingStrings does not properly understand /* style */ comments [auto-close new build system] #1425

Closed
tajmone opened this issue Jan 16, 2017 · 3 comments · Fixed by #2312
Labels
bug package/build Issues relating to npm or packaging

Comments

@tajmone
Copy link
Contributor

tajmone commented Jan 16, 2017

While working on a language definition file I stubmled upon the following error when building HLJS:

Starting build.
Copying documentation.
Writing documentation.
Generating demo.
Building highlight.js pack file.
Compressing highlight.js pack file.
{ task: 'tasks',
  error: { task: 'replaceSkippingStrings', error: 'Unmatched quote' } }

The error was risen by a multiline comments block I've added to the lang file using /* ... */. When I changed the comments to multiple single-line comments (// ...) the error was gone. So I worked to pin down the problem...

I've selectively removed comment lines until I isolated the culprit, but I can't make much sense from what I've found.

The following comment will raise the error:

/*
http://www.someurl.com/
*/

But if I remove the slash / at the end of the line the error dissapears:

/*
http://www.someurl.com
*/

The error also dissapears by adding another slash / at the end of the line:

/*
http://www.someurl.com//
*/

But a third slash bring the problem back again:

/*
http://www.someurl.com///
*/

And it doesn't matter is the slash is the last char on the line or if there are other characters following it — it's the presence of an ODD number of slashes on the line that raises the error!

It seems a problem relating to parsing multiline comments and the way slashes are interpreted — and the presence of two slashes in the URL is what sets into motion. A line with an odd number of slashes seems to raise the error, not so when their number is even. The 'Unmatched quote' error reported is a bit misleading, at the beginning I though it was due to the single quotes, but it wasn't.

The problem doesn't show up when using single line comments:

// http://www.someurl.com/

So we are not talking about an edge-case, similar URLs are common within comment blocks. The error blocks the building process, and doesn't provide much info on where the process failed, so users might need quite some time to isolate the specific line which is causing build failure.

@joshgoebel joshgoebel added package/build Issues relating to npm or packaging bug help welcome Could use help from community labels Oct 7, 2019
@joshgoebel
Copy link
Member

Our build pipeline has custom written code that doesn't properly understand /* */ comments. This is something we really should fix.

@joshgoebel joshgoebel changed the title Error building when lang definition has block comments replaceSkippingStrings does not properly understand /* style */ comments Oct 7, 2019
@joshgoebel
Copy link
Member

If anyone doesn't understand this, ask me. The url issue is that because we don't understand /* comments we think the // in the URL is the start of a comment and that leads to all sorts of problems since really we have no idea what is and isn't commented after that. :)

@joshgoebel joshgoebel self-assigned this Oct 7, 2019
@joshgoebel joshgoebel added this to the New build pipeline milestone Oct 16, 2019
@joshgoebel joshgoebel modified the milestones: Build pipeline, 9.15.12 Oct 24, 2019
@joshgoebel
Copy link
Member

This is a flaw with the existing build system and should by auto-closed if we replace that completely with a new build system (which is the plan).

@joshgoebel joshgoebel added on hold and removed help welcome Could use help from community labels Oct 24, 2019
@joshgoebel joshgoebel changed the title replaceSkippingStrings does not properly understand /* style */ comments replaceSkippingStrings does not properly understand /* style */ comments [auto-close new build system] Oct 24, 2019
@joshgoebel joshgoebel added this to .12 Build in Highlight.js Oct 25, 2019
@joshgoebel joshgoebel moved this from Build System / Packaging to Investigate or close in Highlight.js Oct 25, 2019
@joshgoebel joshgoebel removed their assignment Oct 26, 2019
@joshgoebel joshgoebel modified the milestones: 9.17, 9.18 Dec 6, 2019
@joshgoebel joshgoebel moved this from Promote or close to Has open PR in Highlight.js Dec 7, 2019
@joshgoebel joshgoebel removed the on hold label Dec 7, 2019
@joshgoebel joshgoebel modified the milestones: 9.18, 10.0 Dec 23, 2019
Highlight.js automation moved this from Has open PR to Done Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug package/build Issues relating to npm or packaging
Projects
No open projects
Highlight.js
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants