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

Change scopes in comparison to language-babel #628

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@Ben3eeE
Member

Ben3eeE commented Nov 14, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

  • Add a scope for formal parameter identifiers that is not styled in one-syntax
  • Add unquoted to member_expression > identifier
  • Add component.jsx to jsx components
  • Add spread scope to the spread operator
  • Scope some variables as support.variable.dom

Benefits

Users of language-babel will see more familiar highlighting or can configure it to be more familiar and still get the benefits of tree-sitter parsers

Possible Drawbacks

  • support.variable.dom has the variable class so these will be highlighted as red in one-syntax. This is what TextMate scoped them as except for console which was entity.name.other.console.js or something.

I am open to change the scope for support.variable.dom and move back window and document to be support.variable if someone has a better scope name.

Applicable Issues

Refs #625

/cc: @maxbrunsfeld @atomiks

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Nov 14, 2018

Add unquoted to member_expression > identifier

Actually this change is wrong compared to how language-babel does it. It scopes a and b in:

let c = {a:1, b:2}

as string.unquoted.

While I added unquoted to a in:

c.a = 15;
@atomiks

This comment has been minimized.

atomiks commented Nov 14, 2018

Nice! Any reason why the arrow function change isn't included in this PR?

i.e. both

const fn = () => {};

and

class X {
  fn = () => {};
}

fn should have function scope

scopes: 'support.variable'
},
{
match: '^(window|event|document|performance|screen|navigator|console)$'

This comment has been minimized.

@Arcanemagus

Arcanemagus Nov 14, 2018

Should performance be marked as this scope? As far as I know it's not part of the DOM, but I could quite easily be mistaken here.

console and navigator could also potentially be left out of here, but as they are accessible under the window object I can see leaving them here.

I'd suggest leaving performance as support.variable, but as the TextMate grammar currently marks it as support.variable.dom and it does call the main timestamp a DOMHighResTimeStamp I can definitely see leaving it there.

I just wanted to bring it up in case we were inheriting a bug from TextMate 😉, feel free to leave it there!

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Nov 15, 2018

Contributor

Maybe performance was previously grouped with window and document because it is a browser API (as opposed to something that's available in node and other non-web environments).

I don't have strong opinions either way.

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Nov 15, 2018

Contributor

Let's go with this. We can come back and tweak it further later.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Nov 14, 2018

@atomiks Arrow functions are left out because I can't find a good way to do it. Those would be better to tacle in a separate issue.

@maxbrunsfeld maxbrunsfeld merged commit c7e6e0e into master Nov 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the b3-scope-like-babel branch Nov 15, 2018

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