refactor: allow .json extension on annotations file #836

[mfranzke]

Summary of changes:

As the original implementation by @engelfrost (kudos for that!) accidentally got reverted with commit 9c49a09, we should add that nice refactoring again to ensure that annotations JSON files could get used with the correct filetype / file-ending, and even also advocate for that file-ending from now on.

@engelfrost implementation even also ensured backwards compatibility, so this wouldn't be a breaking change.

The resulting implementation could get reviewed on https://deploy-preview-1402--patternlab-handlebars-preview.netlify.app/?p=viewall-molecules-global&view=annotations and https://deploy-preview-1402--patternlab-handlebars-preview.netlify.app/?p=pages-homepage&view=annotations

There's still a problem with displaying the tooltips on the "standalone" view (in our example for the header), as the general styles from pattern-lab.css aren't included in this view.
This problem is unrelated to the fixes in this pull request and I've opened another pull request for that to fix: #1406

[JosefBredereck]

How do you find stuff like this all the time? That's crazy 😄
One commit with 253 changes, and you know what got removed by that commit.

[mfranzke]

@JosefBredereck … oh, don't ask … ;-) others read novels, I read git commit history …

[JosefBredereck]

I'm a bit on the fence with that. We should let the user decide if he wants to use JS or JSON, even in the future.
Furthermore, we could check if it is possible to require that .js or .json file so that we do not have to strip comments and anything else in that file. Maybe we can change the way it needs to be written to a module.exports format.

[mfranzke]

@JosefBredereck, actually I've realized that we're stripping that var comments part first of all, to add it again on exporting the data:

patternlab-node/packages/core/src/lib/exportData.js

Line 72 in 41b4ec1

'var comments = { "comments" : ' + JSON.stringify(annotationsJSON) + '};';

So another solution might be to skip that whole transition to a pure JSON file and only provide the possibility with using JS (the existing format), but remove the JSON file possibility, as we're even already providing another format with the markdown notation as well.
So another task for this ticket could be still to correct the documentation part anyhow: https://patternlab.io/docs/adding-annotations/#heading-json-example

What do you think?

[JosefBredereck]

No way 😄. In that case, throw away those nasty JSON files 🗑️

[mfranzke]

okay, sounds reasonable to me. I've set this ticket on draft again and will proceed by removing these parts as well as the nasty existing transitions in the codebase, as well as correct the documentation page.

[JosefBredereck]

The whole file you mentioned is a solution, but if we would rework the codebase with today's standards, we would maybe not do it that way.

[mfranzke]

so does it mean that you want to come up with (or at least describe) a solution ?

[JosefBredereck]

Not for now; it would require a more significant rewrite.
This line shows that we already work with modules somewhere.

patternlab-node/packages/core/src/lib/exportData.js

Line 88 in 41b4ec1

'module.exports = { config, ishControls, navItems, patternPaths, viewAllPaths, plugins, defaultShowPatternInfo, defaultPattern };';

But then we also need to adjust the place where we load those annotations again.

What makes me curious. Annotations always have a comments property and no other. Is it possible that they have different properties? If not, this would be another change I would like to implement somehow.

patternlab-node/packages/core/src/lib/exportData.js

Line 72 in 41b4ec1

'var comments = { "comments" : ' + JSON.stringify(annotationsJSON) + '};';

[mfranzke]

No way 😄. In that case, throw away those nasty JSON files 🗑️

@JosefBredereck, on the other hand, besides the strange way to include the JSON data into the website after a transformation, the positive way of using JSON (even only) would be that this adapts the way other data is being maintained in the pattern lab system (like pattern data).

[JosefBredereck]

That is correct. Nodejs is so fast in transforming the data that we could use the approach that is already in this PR. In the end, it makes no difference.

[JosefBredereck]

Another thing I just discovered and never understood: The annotations are written once and visible on all patterns there are available. Why should I want that?
Would it also be helpful to have a list like useIn: [ <list of pattern names> ]?

[mfranzke]

Another thing I just discovered and never understood: The annotations are written once and visible on all patterns there are available. Why should I want that?
Would it also be helpful to have a list like useIn: [ <list of pattern names> ]?

well, they are as global as the global data, but there's no "scoped" or "local" annotations, related to a pattern even only. This even also made me wonder. Probably this would be a nice enhancement / future feature ?

[JosefBredereck]

Another thing I just discovered and never understood: The annotations are written once and visible on all patterns there are available. Why should I want that?
Would it also be helpful to have a list like useIn: [ <list of pattern names> ]?

well, they are as global as the global data, but there's no "scoped" or "local" annotations, related to a pattern even only. This even also made me wonder.

Or I'm just blind. There is the el: ... part which defines to which elements these annotations get added. That was not clear to me 🥲

[JosefBredereck]

Some minor adjustment is standing out after the merge of #1406
https://github.com/pattern-lab/patternlab-node/pull/1406/files#diff-d562b65f94fc7de820db229a45973334e2c8195978fbc7643d1916ed6e63fd19

[mfranzke]

Some minor adjustment is standing out after the merge of #1406 https://github.com/pattern-lab/patternlab-node/pull/1406/files#diff-d562b65f94fc7de820db229a45973334e2c8195978fbc7643d1916ed6e63fd19

good catch, thanks. Just checked in the migrated file.

[JosefBredereck]

PR was released with v5.16.0