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

Add properties to javascript #2004

Closed
wants to merge 6 commits into from
Closed

Add properties to javascript #2004

wants to merge 6 commits into from

Conversation

EdieLemoine
Copy link

Adds .property to Javascript object properties.

Tested here: https://regexr.com/4i9kg

@RunDevelopment
Copy link
Member

Thank you for your work!

However, I don't think that can/want to merge this, at least, as it is now.
The reason for this are false positives. I also tried to implement properties three times and failed for two reasons: comments and conditional (ternary) operators.

Explaination.

Conditional (ternary) operators are a problem because they look like properties.
One might think that just making sure that the supposed property is not preceded by a ? solves this problem but that is not the case (e.g. foo ? bar ? : baz : 123), unfortunately.

/(?<!\?\s*)<property>(?=\s*:)/; // doesn't work

But we could say: "Every property has to be preceded by either , or {".

/(?<=[{,]\s*)<property>(?=\s*:)/; // doesn't work

That would be the correct approach but won't work because of comments:

{
    // foo isn't preceded by `,` or `{`
    foo: 123
}

return true ? // oh no,
  foo : 123;

"Can't we try to also match the comments before properties? Does that help?"

/(?<=[{,](?:\s|<comment>)*)<property>(?=\s*:)/;
// comment might look like this:
/\/\/.*|\/\*[\s\S]*?\*\//;

Unfortunately, no.

// the lookbehind will match...
return true ? // ...right here -> { // some text
    foo : 123;

Maybe a more real-life example:

return condition ?
    // some long, boring,
    // and clever explaination
    foo : bar;

Actually, you could do it by making sure that the comment/[,{] your matching isn't itself part of a comment. However, this requires nested lookbehinds which is something that Prism does not support.

Both conditional (ternary) operators and comments can be dealt with individually but Prism just isn't powerful enough to deal with both of them.

The result

We can either accept false positives (your pattern) or false negatives.

I think that we should discuss which option we want (false positives or false negatives or neither) before merging your PR.

Personally, I think that just leaving everything as is the best option to highlight code consistently. It's just too easy to false positives/negatives to happen and you can easily find examples of this in the Prism code itself.

/cc @mAAdhaTTah @Golmote

@mAAdhaTTah
Copy link
Member

We should look at what false positives look like in practice before deciding which direction we should take on this one.

@RunDevelopment
Copy link
Member

The basic idea of this pattern is that a property is preceded by either a { or a line break. If we extend this to also allow ,s, we'll get decent results.

This requires well-formatted code to keep the false positives at a minimum and comments are still our enemies but it works reasonably well.

// foo is false positive
return reallyLongVariableName ? "foo and some other stuff" + 
	foo : bar;
// b is a false negative
const foo = { a: 6, /* some comment */ b: 7 };

@EdieLemoine I'm sorry for rejecting your idea too early.
This is probably the best compromise between false positives and false negatives we're going to get without changing Prism core.

That being said: I will take over from here on if that's ok. A robust implementation of this will be a little more complex and I also want to adjust the function-variable pattern.


Also, we should discuss which kinds of properties we want to include. @EdieLemoine included all possible property kinds but I think that's too much. I.e. variables inside computed properties will also be highlighted in the property color.

Simple literals (e.g. { foo: 4 }) and single and double quoted strings should be enough. Template strings are going to be difficult because of the interpolation expressions and JS templates (the Prism language).

Thoughts?

@mAAdhaTTah
Copy link
Member

Simple literals (e.g. { foo: 4 }) and single and double quoted strings should be enough.

This seems fair. I'm somewhat of the opinion that adding this generally is a good idea, and that I would prefer false negatives to false positives.

We also need tests.

@RunDevelopment
Copy link
Member

I just noticed that template strings can't be properties, so { `foo`: true } isn't legal JS.

I'll add computed properties for now.

@RunDevelopment
Copy link
Member

While I was at it, I also implemented a sorter pattern for JS variables/literals and changes to class-name pattern to support names outside of \w.

@RunDevelopment
Copy link
Member

Also nice: JS and JSON will now produce about the same result for a valid JSON document; the only difference being :s.

@RunDevelopment
Copy link
Member

Closed in favor of #3099.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants