empress / ember-showdown-prism

https://ember-showdown-prism.netlify.app/
MIT License
11 stars 3 forks source link

add json5, line-numbers, and normalize-whitespace #58

Closed lupestro closed 1 year ago

lupestro commented 1 year ago

Edit from @mansona : this PR has been simplified to add the required config to prism by default 👍

~Before this change, if app.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 of ember-prism components should be additive between addons, isn't deemed wise, then reject the PR. In the abstract, I can imagine a component using ember-showdown-prism that 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 the typescript component 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 was guidemaker itself. That leaves an open question whether typescript and json5 belong 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.)~

netlify[bot] commented 1 year ago

Deploy Preview for ember-showdown-prism ready!

Name Link
Latest commit 1d23b8f23e6c2d6ff2ddb1c7cfa5b2bb33ec0bf0
Latest deploy log https://app.netlify.com/sites/ember-showdown-prism/deploys/6516fc805ebc210008f6ed89
Deploy Preview https://deploy-preview-58--ember-showdown-prism.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

lupestro commented 1 year ago

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 @glimmer/manager and on 3.28 and 4.4 on the absence of @ember/renderer. It failed on embroider optimized saying Ember is not defined. I verified that the same failures also occur when run with the head of the main branch.

mansona commented 1 year ago

embroider-optimised is failing because of a bug in ember-cli-fastboot (that I need to fix 😫) so I am safe to merge this 👍