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 Tokens for Import and Export Names in JS? (Local, Import and Export Names) #2528

Closed
karlhorky opened this issue Aug 26, 2020 · 14 comments
Closed

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Aug 26, 2020

Information

  • Language: JavaScript
  • Plugins: I tried with JS-Extras too

Does the problem still occur in the latest version of Prism? Yes

Description
Would it be possible to add tokens for the import and export names in JavaScript? (see patterns below taken from the import examples and export examples in the ECMAScript spec)

See below - currently they are not receiving any tokenization. To be clear, I'm referring to the local names, import names and export names with names such as v, x, v2 and ns:

Screen Shot 2020-08-26 at 10 48 37

Screen Shot 2020-08-26 at 11 00 05

I also tried using the js-extras language as follows, but that didn't seem to help:

https://codesandbox.io/s/prismjs-forked-wu91d

Code snippet

The code being highlighted incorrectly:

import v from "mod";
import {x as v2} from "mod";
import * as ns from "mod";

export * as ns from "mod";
export {v as x} from "mod";
export {x} from "mod";
export {v as x};
export {x};
@RunDevelopment
Copy link
Member

What's the use case for this?

Also, we usually highlight concepts, so what is the general idea here?

Syntax-wise, from the imports you want all identifiers (e.g. import { x as y } from "mod" both x and y) and from exports, you want the same, so no export declarations (e.g. export const foo;, export default function foo(){}, and export class Foo {}), right?

@karlhorky
Copy link
Contributor Author

karlhorky commented Aug 27, 2020

Use case is being able to be more specific about import names for themes like this:

PrismJS/prism-themes#103


Also, we usually highlight concepts, so what is the general idea here?

Right ok, I suppose the overarching concept is actually "all identifiers". In addition to the variables in the issue description, I would think that a, b, and foo would receive tokens eg. .token.identifier or something:

Screen Shot 2020-08-27 at 11 54 59

As for a concrete use case about that (also irrespective of the import and export rules), the VS Code dark theme currently has a mismatch between the identifier colors here:

Screen Shot 2020-08-27 at 12 03 48

Maybe for import and export names they could be subclassed as .token.import .token.identifier or .token.identifier.import-identifier or similar, in case the styling changes between import identifiers and regular identifiers in a theme.


If it makes sense, I can change this issue to be about "identifiers" in general.

@karlhorky
Copy link
Contributor Author

I suppose another consideration about assigning tokens to identifiers is if it's possible + feasible to handle the other locations where an identifier is used (eg. c, d, e, f, g, h):

Screen Shot 2020-08-27 at 12 31 11

@RunDevelopment
Copy link
Member

[...] the VS Code dark theme currently has a mismatch between the identifier colors here:

Maybe a bit on how VSC highlights variables in JS/TS. VSC uses the TS language server to parse and analyze JS and TS files (among other things). A while ago they introduced sematic highlighting. This means, that variables are not highlighted by syntax alone but with a deeper understanding of the type and other properties of the variable (powered by the TS language server).

This is why VSC can highlight bar as a function even though, syntactically speaking, it's just another variable.
image

VSC can do this not only for the type of a variable (class/function/other) but also for its mutability. Readonly variables/properties will be highlighted with a darker shade of blue:
image

This is incredibly useful because it gives a lot of information to the developer that is not obvious from syntax alone.

However, that's the problem for us. We need more information than just syntax to handle the nested properties correctly. For flat variables, syntax alone is technically enough, but...

if it's possible + feasible to handle the other locations where an identifier is used [...].

No. Tracking variables requires an understanding of scope. This is hard enough with an AST but we only have a token stream to work with. This is one of the things that Prism simply can't do.

@karlhorky
Copy link
Contributor Author

Right yeah, I understand that we won't get to 100% of the capabilities of an editor with type information.

But for the following, maybe regexes / config like the following would be possible:

const a = 1;
let b = 2,
  c = 3;

export const foo = 1;

Config:

	'declaration': {
		pattern: /(const|let|var)\s+([\n\s]*[_$a-zA-Z\xA0-\uFFFF][$\w\xA0-\uFFFF]*([\n\s]*=[\n\s]*[^,;]+)?[\n\s]*[,;])+/,
		inside: {
			'identifier': {
				pattern: /\b((?!const|let|var)[_$a-zA-Z\xA0-\uFFFF][$\w\xA0-\uFFFF]*)\b/,
				alias: 'string'
			},
			rest: Prism.languages.javascript
        }
    }

For the more complex examples, maybe the contents inside the parentheses of function calls could be parsed with a similar pattern to identifier above.

Technically, it's an expression - maybe a regular expression could be developed for that too... 🤔

const d = e * f;

console.log(g.z);

otherFn(h * i);

@RunDevelopment
Copy link
Member

This will be hard because we have to deal with JS' semicolon insertion. Basically, we don't know where a variable declaration ends. To get this right, we'd have to match a single valid JS expression. The formal language of a single JS expression is context-free and cannot be expressed by a regex. We can usually work around this because we usually know the next character after the expression (e.g. a comma or closing bracket).

If we assume semicolons, it's doable but otherwise, ... I doubt it.

We could settle for the first declaration. That's easy and, hopefully, nobody uses these merged variable declarations.

However, even then: Will this be beneficial?
The end goal is to give const variables a different color to more closely resemble VSC's highlighting. But I'd argue that this would hurt readability because const variables will appear differently when used compared to when they're declared.

If I remember correctly, VSC used to highlight all variable declarations in a different color but then dropped it again in the next release (I assume for similar reasons).

@karlhorky
Copy link
Contributor Author

The formal language of a single JS expression is context-free and cannot be expressed by a regex

Will this be beneficial?

const variables will appear differently when used compared to when they're declared

Ok, I can understand that this goes a bit far then. You're right that the benefit is small here and I agree that it would be confusing.

Maybe we stick with importsandexport`s like this issue's description and forget about declarations for now?

import v from "mod";
import {x as v2} from "mod";
import * as ns from "mod";

export * as ns from "mod";
export {v as x} from "mod";
export {x} from "mod";
export {v as x};
export {x};

@RunDevelopment
Copy link
Member

Ok. Just one question before I make the pull: Should as {name} identifiers get a different class?


Also, the motivation for this feature is to more closely resemble VSC's theme, right? But that uses the normal variable color for imports/exports, so how do you intend to use the feature?

image

@karlhorky
Copy link
Contributor Author

karlhorky commented Aug 27, 2020

Right so if you look at the demo, the maybe-class-name token is light green, whereas the imports are light blue:

Screen Shot 2020-08-27 at 18 07 40


Screen Shot 2020-08-27 at 18 07 36

@karlhorky
Copy link
Contributor Author

I suppose an alternative that may work could be (#9cdcfe is the light blue color):

.token.keyword.module + .token.maybe-class-name,
.token.keyword.module + .token.punctuation + .token.maybe-class-name {
    color: #9cdcfe;
}

@karlhorky
Copy link
Contributor Author

Should as {name} identifiers get a different class?

I guess the same token name should be fine?

@karlhorky
Copy link
Contributor Author

Addressed in #2533 🙌 🚀

@RunDevelopment
Copy link
Member

Closed by #2533.

karlhorky added a commit to karlhorky/prismjs-vs-code-dark-theme-fix that referenced this issue Aug 28, 2020
@karlhorky
Copy link
Contributor Author

Thanks @RunDevelopment !

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

No branches or pull requests

2 participants