[styleguide] Fix error when styleguide is shown…

[savetheclocktower]

…before certain grammars are loaded.

Fixes #560.

Identify the Bug

When a user reloads a window that happens to have a styleguide tab open, it’ll usually throw an error during the load process. This happens because styleguide shows code samples for UI elements, and it expects to be initialized in an environment where the grammars it needs for syntax highlighting are already present.

In investigating this, I found a few other bugs and inconsistencies in GrammarRegistry that needed to be fixed.

Description of the Change

1. atom.grammars.addInjectionPoint allows you to add injections for any grammar, even grammars that haven’t been seen or activated yet. It does this by creating a stub object at the given scope name. But some GrammarRegistry APIs weren’t aware that this was a stub, so they were trying to do things with it. Added some guards to fix this. atom.grammars.grammarForId should never return one of these stubs; if the grammar for source.foo hasn’t yet been added, atom.grammars.grammarForId('source.foo') should always be undefined. Added a spec.
2. The GrammarRegistry#onDidAddGrammar and GrammarRegistry#onDidUpdateGrammar callbacks only ever worked for TM-style grammars. I added parallel paths for Tree-sitter grammars, both legacy and modern. The former callback is triggered when a grammar is first added, and the latter callback is triggered when something adds an injection point for a grammar that has already loaded. Added specs for these.
3. Now we can finally write some code that waits for a specific grammar — that’s what CodeBlock needs to do here. But we don’t want it to wait for the first one to come along; we want to wait for the first one that matches the user’s settings around whether or not to use Tree-sitter. But we also don’t want it to wait forever for an ideal candidate!

I had envisioned a method called GrammarRegistry#whenGrammarAdded that would resolve a promise when a grammar of a given scope name was added. But how should it behave?

1. Wait, possibly forever, for (e.g.) a Tree-sitter language-X grammar, even though there’s a TextMate language-X grammar that has already been loaded, because it respects the user’s language mode settings.
2. Ignore all language mode settings and return the first language-X grammar it finds.
3. Balance these two strategies somehow, possibly with a timeout.

I think option 3 might be promising. But if I add this to GrammarRegistry, we have to support it, and I don’t want to support something half-baked that I dreamt up just to fix an issue with styleguide.

So I put a simplified version of whenGrammarAdded into styleguide’s CodeBlock class. It waits for the first text.html.basic grammar to be loaded, then waits another second, then tells GrammarRegistry to pick the best grammar that has already been loaded (taking language mode preferences into account).

Alternate Designs

Described them above.

Possible Drawbacks

The changes I made to GrammarRegistry could possibly have implications for other parts of the core. I verified all the GrammarRegistry specs are passing, but I’ll wait for CI to do the whole suite and tell me if anything else has failed.

I’m of the opinion that anything I touched in GrammarRegistry involved fixing behaviors that were flat-out wrong, so any code that would break as a result of those fixes is itself broken.

Verification Process

Added a new spec to styleguide. Now it has two!

And, as mentioned above, I’ll find out whether anything new in the main test suite is broken.

Release Notes

Fixed a syntax highlighting issue inside the styleguide package

[confused-Techie]

Without doing much as of yet to review this code, just wanting to comment on your very well thought out concerns in the PR description.

I do think waiting some time for a grammar that respects the users settings makes sense. And when that isn't found within a certain time, we instead load any grammar that fits the language.

Since really the only time I can pick a package waiting for a grammar that isn't found right away, will be one of two situations:

1. Pulsar is still loading, and the grammar will be available within seconds
2. The grammar being sought after doesn't exists locally, and no amount of waiting will ever find it

With that in mind, I think it makes sense to wait for say 2 seconds. And if the preferred grammar never appears, then we move on to return any matching grammar or return the fact that no matching grammar exists.

But I do understand the concern of half baked ideas being added into the Grammar registry, But I could imagine other situations where this might be helpful. Such as reloading (or loading) with a Markdown preview open that contains a grammar within a code block, or any other templating language like this. But otherwise for now I don't think it's a bad idea at all to bake this into the only known package that needs it

[confused-Techie]

Got around to testing these changes, and can confirm I was unable to replicate the issue that was originally reported. Fantastic work here, and especially love the comments explaining the logic within styleguide as admittedly the behavior can look strange without knowing the reason.

Otherwise, obviously adding tests that are passing to validate this behavior is a great sign.

I'd say we should be good to merge this one unless you feel there's anything more to add to it