search

PolymerElements / prism-element

Prism-based syntax highlighting
17 stars 19 forks source link

Event model is flaky #26

Open arthurevans opened 7 years ago

arthurevans commented 7 years ago

This is kind of a meta-bug, related to issues reported on the catalog (https://github.com/Polymer/polymer-element-catalog/issues/207) and elsewhere.

Briefly, the ordering of attached can vary based on whether you're polyfilled or not, so the current interaction model for this element is racy.

I think this was an early experiment in loosely-coupled components, but the protocol isn't reliable.

I suggest that we could either make it more reliable, or simply turn prism-element into a prism-behavior that exposes a single highlight(code, language-hint) function. Which would remove the loose coupling benefits, but be fast and reliable.

... Or we tell users they should fire the event, and see if they get results, and if not, wait a rAF and try again.

FredKSchott commented 7 years ago

@arthurevans thanks for investigating & writing this up! That's a bummer that the protocol isn't more reliable in the polyfill. This is the first I've heard of this, but I'm assuming it's non-trivial or impossible to make the polyfill match the spec here? If not, I think it makes sense to build a prism-behavior, and then move this element into maintenance mode with a warning about the race condition and a link to the new behavior.

@notwaldorf is the real brains behind this element though, I'm just the janitor :) wdyt?

arthurevans commented 7 years ago

I don't think we can fix the attached timing in the polyfill (@sorvell or @azakus can probably confirm there). And I suspect that we don't want to encourage this pattern (adding an event listener on the parent), which is not in the spirit of the mediator pattern.

3 solutions, from simplest to most complex:

1) Just import the library (prism-import.html) and use the global. (Downside: might need to copy the tiny bit of language-guessing logic.)

2) Add a behavior that just exposes highlight (including the language-guessing logic).

3) Add behavior as above, but also provide an _addHighlightListener method, so a parent element could provide a drop-in replacement without updating its children, and the <prism-element> could be trivially reimplemented using the behavior.

In addition to the reliability issues, I suspect firing one event per highlight request is less performant than a simple method call, especially for a page with a lot of small code blocks.