Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composable helpers #62

Merged
merged 11 commits into from Jun 24, 2019
Merged

Composable helpers #62

merged 11 commits into from Jun 24, 2019

Conversation

simonihmig
Copy link
Collaborator

@simonihmig simonihmig commented May 31, 2019

This is a spike at implementing some of the ideas of #56 (comment) and #61 (comment):

  • removes all hightlight.js code as it de-scopes the concern of code highlighting
  • adds a get-code-snippet helper exposing a { source, language, extension } POJO
  • moved most implementations into private plain JS functions, some of which could be exposed as public importable functions in addon/index.js for use cases where those are needed in JS land rather than only in templates
  • retained the previous code-snippet component (again without any highlighting) for straightforward rendering, but now implemented based on the helper. This could be removed, as it is just for convenience.
  • added a helper-based implementation of the dynamic replacement functionality initially implemented in Add support for replacement of dynamic properties #61, for now to play with the overall design. This could be extracted into a separate addon. Although I am not sure how useful that is, as the replacement logic is based on the assumption that the code template is rendered in a template (replacement of {{...}} instances), so I think the use case is pretty much coupled to the functionality of ember-code-snippet... 🤔
  • added a basic playground in the dummy app to play with the use cases, including an example of highlighted code using ember-prism
  • moved the module containing the extracted snippets to ember-code-snippet/snippets instead of <app-name>/snippets (merged into treeForAddon now), to make it easily importable from the static JS modules (i.e. not knowing the app's name). AFAICT that module should be considered private, although it has been used in the wild: https://github.com/ember-learn/ember-cli-addon-docs/blob/master/addon/components/docs-snippet/component.js#L4. But these are major breaking changes anyway, so should be ok!?

To Do:

  • finalize public API (public exports, include component?, POJO structure etc.)
  • add tests
  • update docs

Copy link
Owner

@ef4 ef4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going the right direction, thanks.

I'm fine with moving the snippets themselves into treeForAddon, I agree it's a better place for them.

I agree that we don't really need a code-snippet component and may want to leave that name free to be used by the other addon that implements syntax highlighting.

@simonihmig
Copy link
Collaborator Author

Thanks @ef4 for the review! I pushed the following update:

  • removed component
  • added helper tests
  • updated docs
  • added an export for a getCodeSnippet function, so you can have the same functionality of the helper in JS land (including docs + tests)

The only remaining question is how we should handle the actual feature that kicked off all of this: the dynamic replacement...

added a helper-based implementation of the dynamic replacement functionality initially implemented in #61, for now to play with the overall design. This could be extracted into a separate addon. Although I am not sure how useful that is, as the replacement logic is based on the assumption that the code template is rendered in a template (replacement of {{...}} instances), so I think the use case is pretty much coupled to the functionality of ember-code-snippet

Again, I don't really have a good use case for having that replacement helper without this addon, and as the code is pretty simple, it feels a bit as an unnecessary overhead to extract that into its own addon. But I am fine with a decision either way...

@simonihmig
Copy link
Collaborator Author

The only remaining question is how we should handle the actual feature that kicked off all of this: the dynamic replacement...

I removed this for now, to unblock this. We can later add it back, either here in a separate PR, or as another package...

@ef4 As there were quite a few changes after your initial review, and as this changes the public API radically, I would love your final approval before merging this! 🙏

README.md Outdated
includeHighlightStyle: false
});
```hbs
{{#code-block language="handlebars"}}{{get (get-code-snippet "demo.hbs") "source"}}{{/code-block}}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use an example like this?

{{#let (get-code-snippet "demo.hbs") as |snippet|}}
  {{#code-block language=snippet.language}}
    {{snippet.source]}
  {{/code-block}}
{{/let}}

It would show why we decided to return a POJO with the language too. This snippet can more easily generalize to a reusable component that accepts the snippet name as an argument:

{{!-- templates/components/code-snippet.hbs --}}
{{#let (get-code-snippet @name) as |snippet|}}
  {{#code-block language=snippet.language}}
    {{snippet.source]}
  {{/code-block}}
{{/let}}

Which you would use like:

<CodeSnippet @name="demo.hbs" />

Maybe we should even suggest that pattern in the README. I think most apps that will have many snippets should make a component like this that puts together their chosen syntax highlighter and get-code-snippet.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and also we should explain this is the upgrade path for people who were using the current version of the addon. If they just want to keep the behavior they had, they should make their own <CodeSnippet /> component to replace the one we removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, updated according to your suggestion!

It would show why we decided to return a POJO with the language too

There is one potential problem though: that example would actually not work properly with prism.js and hbs templates, as our helper returns 'htmlbars' as the language, while prism expects 'handlebars'. But as that the language value is not standardized across highlighters, there is not so much we can do about it!?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If prism is going to be our recommended syntax highlighter, I'm fine with changing our helper to return handlebars.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized I committed my recent Readme updates to the wrong branch. Fixed! 🤦‍♂️

If prism is going to be our recommended syntax highlighter, I'm fine with changing our helper to return handlebars.

Ok, agree. Updated that, including a test + docs.

@simonihmig simonihmig changed the title WIP: Composable helpers Composable helpers Jun 23, 2019
@ef4 ef4 merged commit 46a9f62 into ef4:master Jun 24, 2019
@ef4
Copy link
Owner

ef4 commented Jun 24, 2019

Published as 3.0.0.

@jrowlingson
Copy link

I removed this for now, to unblock this. We can later add it back, either here in a separate PR, or as another package...

@simonihmig Are you still planning to PR this change?

@simonihmig simonihmig deleted the composability branch January 2, 2020 22:27
@simonihmig
Copy link
Collaborator Author

@jrowlingson no, I chose for now to implement this just for myself here: https://github.com/kaliber5/ember-bootstrap/blob/master/docs/app/helpers/dynamic-code-snippet.js. But feel free to reuse it, if this is what you are looking for!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants