search

johno / ember-remarkable

Ember addon for Remarkable markdown parsing helpers and components.
MIT License
26 stars 20 forks source link

added plugin support (possibly #7) #29

Closed maciek134 closed 7 years ago

maciek134 commented 7 years ago

I started working on it before seeing that you wanted to define plugins in App. I believe this is a better solution as it allows to, for example, create plugins specific to a controller / route.

I don't have experience testing addons (ember test wouldn't work for me) so there is just an example plugin in controllers/application.js.

If you want me to make it possible to define plugins in App just reject that PR and let me know, should be too hard.

johno commented 7 years ago

Thanks @maciek134! This is looking great. I will take it for a spin this weekend and get it merged and published!

johno commented 7 years ago

One thing we might want to change is perhaps making plugins defined globally in the app config and then being able to toggle them off/on in props passed to the component, but I will do some more thinking on that when I can get around to this tomorrow. I will provide more in depth feedback then.

And thank you again for opening up the pr 🙇‍♀️ !

maciek134 commented 7 years ago

Close / re-open seems to have fixed CI ;p I could make it work in both ways - so you can define plugins in either app config or controller / route. I need the latter, because dynamic rendering doesn't work for me in Ember 2.10.0 - it's like there is no data available for the component.

Scratch that, there was no data because none was provided to the component anywhere. I'll work on moving plugins to app config.

maciek134 commented 7 years ago

Ok, I think I tried every way possible and plugins can't be stored in any config (be it app/config/environment.js or ember-cli-build.js) without resorting to some ugly hacks. Reasons: functions can't be stored in environment.js, because it's contents are compiled to JSON; anything in ember-cli-build.js is only available during compilation - here it probably could be hacked by creating some global with unique name and injecting it later in initializer.

I also realized that plugins for remarkable should be injected using md.use(), not how I implemented that.

Let me know if the hacky way is ok, that'll take a few lines of code to implement.

johno commented 7 years ago

Thanks for looking into it. I didn't realize we wouldn't be able to pass in the plugins in ember-cli-build. I think we can avoid global plugin config (it was a "nice to have" anyway). My apologies for sending you off in the wrong direction 😬 .

So let's roll with this solution, where a wrapper component or controller can define the plugins to pass in to md-text. Though, before merge and release I'd like to add in a quick test and leverage md.use(). If you would like some help with creating a test please let me know!

✨ ✨ Thanks!

maciek134 commented 7 years ago

I'll make changes tomorrow morning, I've got the tests figured out.

maciek134 commented 7 years ago

For some reason ember-beta check fails on CI (even though ember-canary passes).

johno commented 7 years ago

For some reason CI has been a little flaky (I think since the version of ember-cli is pretty out of date in this project). After restarting the build for beta it has now passed. So, I'll go ahead and merge/release now.

Thank you! 🎉

johno commented 7 years ago

Published to npm as ember-remarkable@3.4.0. Let me know if you encounter any issues!