add json5, line-numbers, and normalize-whitespace #58
Conversation
✅ Deploy Preview for ember-showdown-prism ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
index.js
Outdated
| 'apacheconf', | ||
| 'bash', | ||
| 'css', | ||
| 'handlebars', |
There was a problem hiding this comment.
we don't want handlebars -- because we don't use this syntax (see the removed list, it's not present)
There was a problem hiding this comment.
The Git history is correct - I built from after your change. but, I copy/pasted a chunk of code from where I was debugging it over in the guides, which hadn't picked up your change yet. Sorry about that. I went back and double-checked everything else in this file and it doesn't look like I missed anything further.
index.js
Outdated
| const existingComponents = app.options['ember-prism']?.components ?? []; | ||
| const newComponents = existingComponents.slice(); | ||
| for (const component of desiredComponents) { | ||
| if (!existingComponents.includes(component)) { | ||
| newComponents.push(component); | ||
| } | ||
| } |
There was a problem hiding this comment.
If this logic is only needed if there are app.options['ember-prism'] then let's move it to the appropriate branch below.
There was a problem hiding this comment.
I had figured that, by ensuring the only significant block of code was always executed, we guaranteed it would always be well tested, especially where this code was in a difficult place to test independently of normal operation. I was killing risk, which is usually my first consideration, by exposing it to light. With only a dozen or so items, performance doesn't enter into it in any significant way. (I had actually had it the other way first.)
However, I can move it around. Sure.
index.js
Outdated
| newComponents.push(component); | ||
| } | ||
| } | ||
|
|
||
| if (!app.options['ember-prism']) { |
There was a problem hiding this comment.
Since we're adding an else block, let's switch the branches around to avoid "if not, else"
There was a problem hiding this comment.
That makes good sense.
|
As per Chris Manson's indication of direction, I reverted all of my changes, just added json5 and the plugins that guidemaker was supplying. Once this change is in play, we can remove the code from guidemaker that was overriding this. (Once I'm absolutely sure the work I did won't need to be reinstated, I'll also squeeze out all the intermediate revisions.) I doubt that this will pass all ember-try tests - I ran ember-try on it locally and it failed on 3.16, 3.20, and 3.24 for the absence of |
mansona
changed the title
|
embroider-optimised is failing because of a bug in ember-cli-fastboot (that I need to fix 😫) so I am safe to merge this 👍 |
Edit from @mansona : this PR has been simplified to add the required config to prism by default 👍
Before this change, ifapp.options['ember-prism']has been initially set up by another add-on, the components this addon wanted to use would be ignored. Now it adds any that aren't already there.If the central idea of the PR, that the set ofember-prismcomponents should be additive between addons, isn't deemed wise, then reject the PR. In the abstract, I can imagine a component usingember-showdown-prismthat wants to narrow the range of prism components from 14 to perhaps two or three to keep the runtime size down. With this PR, they would no longer be able to do so. In practice, I doubt that's likely to be a serious concern, but I leave that to the maintainers to decide. They may know of cases where it applies.The original impetus for this fix was that thetypescriptcomponent had been added here and was being ignored when providing typescript examples in the guides. (We also wanted to add json5 examples.) As it turns out, the add-on that was setting a list that pre-empted this one wasguidemakeritself. That leaves an open question whethertypescriptandjson5belong in this list or that one.Since typescript is now more central to Ember work in general, I think the typescript component, at least, probably belongs in the base list for this add-on next to javascript. json5 might be more debatable. It's definitely slated for use in the Typescript portion of the guides. We can pull json5 from the PR and move it to the list in guidemaker if desired. If this PR is accepted, guidemaker could either specify precisely what components the guides use or trim its list to things it wants to add. (I suspect they both have the same maintainer. Right, Chris? ;) However you want to architect it.)