Add diffing logic #18
Conversation
| const fileWriters = { | ||
| 'frontmatter-md': writeFrontmatterMarkdown, | ||
| json: writeJSON, | ||
| yml: writeYAML | ||
| }; |
There was a problem hiding this comment.
@eduardoboucas you can use this battle-tested utility stringifyDataByFilePath to write all sort of files:
https://github.com/stackbithq/utils/blob/d1222b63ede765fb895668b641cb3f44c238d6ea/src/index.js#L373-L394
It takes into account several quirks that will prevent the app from crashing and store files safely.
There was a problem hiding this comment.
I'd rather not go through the trouble of changing the implementation unless something needs fixing. The code you linked looks good, but we have slightly different requirements (e.g. we don't need to choose the writer based on the file path, that is provided to us by the user). Also we're using a different YAML package, which a different syntax.
So if you have any concerns about a specific part of the existing implementation, can you flag those specifically?
There was a problem hiding this comment.
Ok, then just look at the YAML implementation in the link I've sent you and add noRefs attribute to make sure that if an object has circular references it won't be split. Also what about supporting TOML and markdown files with TOML and YAML frontmatters? I know Hugo and 11ty make use of these.
| const { __diff, ...currentDataForPlugin } = data; | ||
| const previousData = this.dataForPlugin[index] || initialData; | ||
| const diffs = Object.keys(currentDataForPlugin).reduce((diffs, dataBucketKey) => { | ||
| return { | ||
| ...diffs, | ||
| [dataBucketKey]: generateDiff(previousData[dataBucketKey], currentDataForPlugin[dataBucketKey]) || [] | ||
| }; | ||
| }, {}); | ||
|
|
||
| this.dataForPlugin[index] = currentDataForPlugin; | ||
|
|
||
| const transformedData = await plugin.transform({ | ||
| data: { | ||
| ...data, | ||
| __diff: diffs | ||
| }, | ||
| debug: this.getDebugMethodForPlugin(pluginName), | ||
| getPluginContext: () => contextSnapshot[pluginName] || {}, | ||
| log: this.logFromPlugin.bind(this), | ||
| options: this.parsePluginOptions(plugin, pluginBlock.options) | ||
| }); | ||
|
|
||
| return transformedData; |
There was a problem hiding this comment.
It looks like the data is passed to plugin transform functions by reference and also stored by reference in dataForPlugin. This means that if one plugin mutates the data, it will be changed in dataForPlugin as well, and therefore affect the next diff iteration.
Example:
data = {
objects: [{"foo": "bar"}]
}
=> currentDataForPlugin = {objects: [{"foo": "bar"}]} // same object
=> this.dataForPlugin[index] = {objects: [{"foo": "bar"}]} // same object
plugin.transform({data: {...data}})
Plugin code
transform({data, ...}) {
data.objects.foo.bar = "changed";
}
This changes the value inside the data passed in transform but also the value inside this.dataForPlugin[index].
Next time transform runs, the previousData for that plugin will have the value changed and not the original value.
The data passed to transform function (or the data saved into dataForPlugin) should be deeply cloned. Or better yet, deeply freeze the data before passing it to plugin transform (or use immutable.js), requiring plugin developers to always return new objects by leveraging functional development and use of pure functions.
There was a problem hiding this comment.
That's true. I did consider this when I was working on the implementation, but I worry that by freezing/cloning the data we'll be adding too much effort. We already clone the entire data object on every transform, now we're talking about doing it also for every plugin, on every transform.
I mean, we can do it if you feel that it's important, but it seems that we've been on the fence about the importance of the whole diff object concept, so I'm a bit weary about making it considerably more expensive. On the other hand, there's the argument that if we're doing it, we might as well do it right! 😄
What do you think?
There was a problem hiding this comment.
"On every transform" you mean every transform of every plugin? Or every global transform run?
If this is for global transform run, then the impact is low and there is no problem with that.
But if the diff is run for every plugin, then my question is do we need to compute the diff for every plugin? Or can we run it once before we begin calling transform functions?
There was a problem hiding this comment.
Anyway, we should check if deep-freeze has any significant performance impact. If not, then I would safely use it.
No description provided.