Upgrade to latest slate #554

PeterKottas · 2018-08-12T11:34:06Z

This work is not ready yet.
I am just opening PR for convenience so that you can see my changes and have a chance to react.
This of course has to do with #553
Right now I went with creating slate-next. The idea is to either at the end delete old slate and rename this back to slate. That is of course if we manage to make it work with old documents.

Signed-off-by: Peter Kottas <petokottas@gmail.com>

PeterKottas · 2018-08-12T15:19:56Z

Ok this version appears to be working quite well. @arekkas please give it a go when you have a chance. Of course there's some more work to do (consider with to do with the "slate"&"slate-next" situation) and fix the example content but other than that, it appears to be working.
Also the fucking sign-off is not working again. I think it's vs-code doing something to it. The fix you suggested in #552 doesn't work either

PeterKottas · 2018-08-13T00:10:41Z

Complete list of changes in the underlying serialized data

kind is renamed to object
ranges is renamed to leaves
whole content is wrapped in additional document node

I think we could make it backwards compatible by hacking around with https://github.com/PeterKottas/editor/blob/master/packages/plugins/content/slate-next/src/hooks.js#L130
Namely we could update plugin version, check in that place if it's the old version, and if so, just perform these actions on the serialized document. Not that importFromHtml way of storing data doesn't suffer from this problem.

Signed-off-by: Peter Kottas <petokottas@gmail.com>

aeneasr · 2018-08-13T15:58:16Z

Do you think it would be possible to migrate those changes to the slate plugin itself and offer a way for migrations? We do have a way to check for plugin versions iirc for this specific use case.

PeterKottas · 2018-08-13T16:00:12Z

Yeah absolutely, it's already implemented in #557 ;)

PeterKottas · 2018-08-13T16:01:04Z

Just need to wait for that branch to merge, then merge it into this and make the slate-next -> slate renaming, it should then work perfectly with old data. I've also added some unit tests for migrations

aeneasr · 2018-08-13T16:02:44Z

Ok I see, merging unstable/dev work into master is not a good practice so I created a new branch https://github.com/ory/editor/tree/slate-next where both PRs can be merged into (please change base of both) - then review it and then finally merge into master. Sound good?

PeterKottas · 2018-08-13T16:08:29Z

Will I be able to commit directly to it?

PeterKottas · 2018-08-13T16:09:41Z

If not I can merge my 2 together and PR against that new branch. I just wanted to keep those separate as the migrations are fairly self contained thing

aeneasr · 2018-08-13T16:13:03Z

You don't have write access to this repository at the moment but if you keep up this amazing work you should! If you rebase both PRs against that branch I can merge them right now and you can create another PR against that new branch with the merged changes to make your latest changes. Alternatively you can merge your two branches on your fork and create a new PR. Whatever you like

aeneasr · 2018-08-13T16:13:24Z

I can also merge the two PRs on the new branch and you fork it and work on that base

PeterKottas · 2018-08-13T16:16:28Z

Cheers man, yeah I am in a rush to add CMS to our system and as we love ory editor, we have decided to give some love back ;) For now, I'll be happy to merge my 2 together, that will be straight forward. It however makes sense to me to keep the PR for migrations alive, just for your convenience. That way, you can easily see (and discuss) in separation how I implemented migrations. Sounds good?

aeneasr · 2018-08-13T16:18:14Z

Sound good! I won't be able to review until end of the week though. The PRs are rather large and it's hard to see what actually changed in this one as it's a completely new code base.

PeterKottas · 2018-08-13T16:19:48Z

Yeah I know and understand. Don't worry about it. Just as soon as possible will do ;) Thanks for letting me know about time schedule

PeterKottas · 2018-08-13T16:21:33Z

From now on, this branch contains work on migrations.

Signed-off-by: Peter Kottas <petokottas@gmail.com>

aeneasr · 2018-08-16T20:38:56Z

packages/plugins/content/slate/src/hooks.js

+        nodes: [
+          {
+            object: "block",
+            type: "paragraph",


Is it intentional that this is "paragraph" and not P?

nope that's an error on my side, fixed

aeneasr · 2018-08-16T20:40:42Z

packages/plugins/content/slate/src/migrations/v002.js

+import { Migration } from 'ory-editor-core/lib/service/plugin/classes';
+import rename from 'deep-rename-keys';
+const migration = new Migration({
+    version: '0.0.2',


I think it would be better to define the version ranges here:

new Migration({ toVersion: '0.0.2', fromVersionRange: '~0.0.1' // just an example, migrate: state => state })

This is actually not how it works right now, it's little bit complicated like always with migrations. I don't even believe there's right way to do this.
Basically right now, you as an author should provide all migrations necessary to migrate from older to newer.
These are sorted by version. That means that if I define migrations for
0.0.1
0.0.3
0.0.6
My plugin is version 0.0.6 for example.
My content is version 0.0.2
Then it would apply migration 0.0.3 and 0.0.6
If content was 0.0.0
It would use all 3 migrations.
If I explained this properly, it should make sense that the "fromVersionRange" is not necessary. It's implicitly based on all migrations provided.

That's not how semver works though, that's how database migrations work. There you have a counter (e.g. migration_count) which increases by one when you apply a migrations. It looks like you're doing this here too.

Semver, or more specifically library releases, work differently. 1.0.0 might be compatible with 0.8.0 but not 0.9.0 because 0.9.0 could a later release than 1.0.0 (think maintenance release). If you were to provide migrations for all versions, you'd have to retroactively upgrade the versions which is not possible as you can't tag twice in git (without breaking serious stuff). That's why we have to constrain the versions a certain migration applies to. Chaining migrations is more challenging here for the developer writing the plugin, but it is doable. From the API standpoint though, it is more concise and compatible with semver or more generally npm releases/git tags.

aeneasr · 2018-08-16T20:45:52Z

packages/plugins/content/slate/src/plugins/lists.js

@@ -24,7 +24,6 @@
 import React from 'react'
 import ListIcon from '@material-ui/icons/List'
 import OrderedListIcon from '@material-ui/icons/FormatListNumbered'
-import createListPlugin from 'slate-edit-list'


Is slate-edit-list not compatible with the latest slate or what is the reason for removing it?

I had to modify this plugin by borrowing the implementation from official docs. They didn't use it there and I didn't find anything it did. Also I think it might have been incompatible. But tbh I've change so many things, I don't exactly remember. I've tested lists and they seem to work. Not sure if there's some functionality missing

The plugin has key bindings and validators (see https://github.com/GitbookIO/slate-edit-list#features). Those are definitely missing!

aeneasr · 2018-08-16T20:46:55Z

packages/plugins/content/slate/src/plugins/paragraph/node.js


 class Paragraph extends Component {
-  shouldComponentUpdate(nextProps) {


A particular reason for removing shouldComponentUpdate?

The way react decides if to update component is actually shallowEqual so it seemed redundant to do it twice. Unless there's something I am missing.

You're right, not sure what the point of it was.

aeneasr · 2018-08-16T20:48:37Z

packages/plugins/content/slate/src/plugins/paragraph/node.js

    const align = this.props.node.data.get('align')
    return (
      <p {...attributes} style={{ textAlign: align }}>
-        <Placeholder
-          className="ory-plugins-content-slate-paragraph-placeholder"


Particular reason for not keeping the classname?

They removed the placeholder from slate. The placeholder is now provided as parameter to the editor.

aeneasr

I reviewed the whole PR now. Looks very solid. The biggest issue I see is how migrations are working at the moment. Migrations will be a huge and defining part of the API and should be thought through. I added some suggestions as to how a migration could look like. Open for suggestions!

Signed-off-by: Peter Kottas <petokottas@gmail.com>

PeterKottas · 2018-08-16T21:22:07Z

Yeah I know about suggestions. That's why I initially wanted to keep it as separate PR, to make it easier to modify stuff and reason about things.

Ok here's my 2 cents about migrations. Bear in mind it's very complex topic like you pointed out and it's kind of hard for me to explain. I'll do my best.

In the context of this library, I don't consider migrations to be "key" feature. Think of it this way, you have thousands of people using this lib, and so far it wasn't there at all. I am not saying this to force you to accept wrong solution to the problem, just pointing out it will probably not be used by too many people.
Migrations here are definitely not something like in entity framework where you end having hundreds of migrations for one project.
Here I assume 1 migration (if that) will be more than enough for most plugins. It's important to point out that even when plugins change quite a lot, it's not exactly hard to make them backwards compatible.
I think the slate example was fairly extreme case.
That's why I designed it the way I did.
The "version" in migration merely says to what version it upgrades.
The "fromVersion" (or version range) here is not necessary as we are always expecting all of the migrations to be there. (coincidentally entity framework migrations works exactly the same)
For a known content version, you just take all migrations where semver > than content version. Up to the version of plugin that you use. And that's it.
It's actually easier than I though :D

PeterKottas · 2018-08-16T21:23:10Z

Also on a side-note, I've implemented other feedback from PR (most of it definitely pretty straight forward compared to the migration talks)

Signed-off-by: Peter Kottas <petokottas@gmail.com>

aeneasr · 2018-08-16T22:28:04Z

In the context of this library, I don't consider migrations to be "key" feature. Think of it this way, you have thousands of people using this lib, and so far it wasn't there at all. I am not saying this to force you to accept wrong solution to the problem, just pointing out it will probably not be used by too many people.

I disagree. Migrations of the document model are as important as migrations of a SQL Schema. If we significantly break the document model without a way for migrating to a newer version, it will be a terrible developer experience. The system was not in place because there was no need in the core library, but versioning existed from the beginning due to this.

Versioning for libraries is, unfortunately, more complex due to the way typical libraries are versioned (semver). This allows for high complexity when releases are not sequential (e.g. 0.9.0 released after 1.0.0 but 0.8.0 released before 1.0.0).

If you don't want to take time to think about this - which would be totally acceptable - I'll free up some time to do so.

PeterKottas · 2018-08-16T22:41:09Z

(e.g. 0.9.0 released after 1.0.0 but 0.8.0 released before 1.0.0) is a very good point. Yeah I sort of agree with you. For now, I am tempted to leave it for another PR if that would work for you. The only thing I am afraid of right now is to wait a long time to merge this. If we can in fact merge it, I'd be happy to work with you on improving the migrations after this goes in.

aeneasr · 2018-08-17T07:37:00Z

The only thing I am afraid of right now is to wait a long time to merge this. If we can in fact merge it, I'd be happy to work with you on improving the migrations after this goes in.

I created a 0.4.0 branch where we can merge this as work in progress. Once the changes are done we'll push it to master and tag it v0.4.0 . That way it's easier to work on this branch and also use it in npm. Deal?

PeterKottas · 2018-08-17T11:16:33Z

Yeah sure, that works for me. So will you just merge this now, release it and create new PR against master? Did I get it right? The only problem then will be I won't be able to commit directly to it. But I can PR against it if that's preferable.

PeterKottas · 2018-08-17T12:16:18Z

In the meantime, I think I might have cracked the migrations and do it properly this way.

Let's assume that all migrations necessary to migrate to current version are present in code. This is an important assumption and it should become guideline for developers. Let's call it assumption A

For the argument sake, let's create this example situation.

Existing plugin versions in order of releases

0.1.0 -> 0.2.0 -> 0.3.0 -> 1.0.0 -> 0.4.0 -> 1.1.0

We would then add the param you proposed so the migration object could look like:

export type Migration = {
    toVersion: string, // SEMVER, single version
    fromVersionRange: string // SEMVER range
    migrate: (state: Object) => Object
}

Sidenote, once/if we have a proper typesystem in place, we could also keep all state types and provide 2 generic arguments

export type Migration<FromState, ToState> = {
    toVersion: string, // SEMVER, single version
    fromVersionRange: string // SEMVER range
    migrate: (state: FromState) => ToState
}

Which would imho be pretty cool

Let's now create the migrations for our example

[
  {
    toVersion: '0.3.0',
    fromVersionRange: '0.1.0 – 0.2' // >=0.1.0 to < 0.3.0 according to https://devhints.io/semver
  },
  {
    toVersion: '1.0.0',
    fromVersionRange: '~0.3.0' // >=0.3.0 to < 0.4.0 according to https://devhints.io/semver
  },
  {
    toVersion: '0.4.0',
    fromVersionRange: '~1.0.0' // >=1.0.0 to < 1.1.0 according to https://devhints.io/semver
  },
  {
    toVersion: '1.1.0',
    fromVersionRange: '~0.4.0' // >=0.4.0 to < 0.5.0 according to https://devhints.io/semver
  }
]

Let's do this for content with version 0.1.0 and plugin version 1.1.0

Pseudo code

**currentDataVersion** = '0.1.0'
**pluginVersion** = '1.1.0'
while **currentDataVersion** != **pluginVersion**
  **migration** = Find migration that upgrades from **currentDataVersion** based on **fromVersionRange**
  **state** = apply **migration**
  **currentDataVersion** = **migration**.**toVersion**

Based on A we can guarantee that after going through that while loop, even if we don't end up with exactly the same version as our pluginVersion, it should still mean that we have the correct data.

And that's pretty much it. Not complicated at all and if we agree on this, I can implement it later on today.

aeneasr · 2018-08-17T12:34:22Z

Yeah sure, that works for me. So will you just merge this now, release it and create new PR against master? Did I get it right? The only problem then will be I won't be able to commit directly to it. But I can PR against it if that's preferable.

I will do another review pass, then merge

And that's pretty much it. Not complicated at all and if we agree on this, I can implement it later on today.

Code example looks good. I think that's the way to do it!

PeterKottas · 2018-08-17T13:28:10Z

I was trying to fix the lists, but weirdly, I noticed it doesn't work properly on the master either. Check out https://editor.ory.am/?utm_source=github&utm_medium=header&utm_campaign=editor, select one of the existing lists and notice that the button for list is not active. If you click it, it will wrap it once again.

The reason for this is it's wrapped in additional paragraph. Not sure how that happened. Do you ahve any insight?

If you create a new one, it works as expected, it's only the ones baked in content.js

If you check out the html, you'll notice the extra paragraph

aeneasr · 2018-08-17T14:00:55Z

I’ll check it tomorrow

…

On 17. Aug 2018, at 15:28, Peter Kottas ***@***.***> wrote: I was trying to fix the lists, but weirdly, I noticed it doesn't work properly on the master either. Check out https://editor.ory.am/?utm_source=github&utm_medium=header&utm_campaign=editor, select one of the existing lists and notice that the button for list is not active. If you click it, it will wrap it once again. The reason for this is it's wrapped in additional paragraph. Not sure how that happened. Do you ahve any insight? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Signed-off-by: Peter Kottas <petokottas@gmail.com>

PeterKottas · 2018-08-17T20:48:27Z

I am not exactly sure how you suppress build errors like this one. What method you prefer/use?

aeneasr · 2018-08-18T14:43:26Z

We can ignore that for now!

PeterKottas · 2018-08-18T14:57:02Z

Ok, fair enough ;)

Signed-off-by: Peter Kottas <petokottas@gmail.com>

PeterKottas · 2018-08-24T21:58:21Z

Hey mate, is this not released yet? I am thinking maybe the flow errors broke the release pipeline?

aeneasr · 2018-08-25T10:00:03Z

Yeah the lerna upgrade broke it, fixed now

PeterKottas added 4 commits August 11, 2018 19:27

Basic setup of new plugin

180d593

Some progress on depracted stuff

7140dd1

Upgrading plugins

431c84e

Finishing touches

8bd2ddf

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Fiex html serializing and content

f4fb098

Implemented migrations

b5dc40d

Signed-off-by: Peter Kottas <petokottas@gmail.com>

PeterKottas mentioned this pull request Aug 13, 2018

Implemented migrations #557

Closed

PeterKottas added 2 commits August 13, 2018 17:10

Fixed type error

9c1c5c5

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Fixed unit tests

a8b0ffd

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Merge branch 'plugin-versions'

7e02796

PeterKottas added 6 commits August 13, 2018 20:29

Got rid of slate-next and added mutations

fad759e

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Fixed tests

6c40c09

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Flow issues fixes

7e5cc5c

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Types fixes

a3fb180

Types

bfa1bae

Fixing some warnings

559ae8b

Signed-off-by: Peter Kottas <petokottas@gmail.com>

aeneasr reviewed Aug 16, 2018

View reviewed changes

PR Feedback

239f7de

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Type problems

7666650

Signed-off-by: Peter Kottas <petokottas@gmail.com>

aeneasr changed the base branch from master to 0.4.0 August 17, 2018 08:26

PeterKottas added 2 commits August 17, 2018 22:33

Updated migrations

52f753e

Signed-off-by: Peter Kottas <petokottas@gmail.com>

Reverted lists changes

858b3d5

Signed-off-by: Peter Kottas <petokottas@gmail.com>

aeneasr merged commit 913fc7b into react-page:0.4.0 Aug 18, 2018

PeterKottas mentioned this pull request Aug 19, 2018

Error when initializing #559

Closed

aeneasr pushed a commit that referenced this pull request Aug 22, 2018

plugins/slate: Updates to latest slate and adds migrations (#554)

94a607c

Signed-off-by: Peter Kottas <petokottas@gmail.com>

PeterKottas mentioned this pull request Dec 7, 2019

Support for migrations ianstormtaylor/slate#3272

Closed


		class Paragraph extends Component {
		shouldComponentUpdate(nextProps) {

Upgrade to latest slate #554

Upgrade to latest slate #554

Conversation

PeterKottas commented Aug 12, 2018

PeterKottas commented Aug 12, 2018

PeterKottas commented Aug 13, 2018

aeneasr commented Aug 13, 2018

PeterKottas commented Aug 13, 2018

PeterKottas commented Aug 13, 2018

aeneasr commented Aug 13, 2018

PeterKottas commented Aug 13, 2018

PeterKottas commented Aug 13, 2018

aeneasr commented Aug 13, 2018

aeneasr commented Aug 13, 2018

PeterKottas commented Aug 13, 2018

aeneasr commented Aug 13, 2018

PeterKottas commented Aug 13, 2018

PeterKottas commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

PeterKottas commented Aug 16, 2018

PeterKottas commented Aug 16, 2018

aeneasr commented Aug 16, 2018

PeterKottas commented Aug 16, 2018

aeneasr commented Aug 17, 2018

PeterKottas commented Aug 17, 2018

PeterKottas commented Aug 17, 2018 • edited

aeneasr commented Aug 17, 2018

PeterKottas commented Aug 17, 2018 • edited

aeneasr commented Aug 17, 2018 via email

PeterKottas commented Aug 17, 2018

aeneasr commented Aug 18, 2018

PeterKottas commented Aug 18, 2018

PeterKottas commented Aug 24, 2018

aeneasr commented Aug 25, 2018

PeterKottas commented Aug 17, 2018 •

edited

PeterKottas commented Aug 17, 2018 •

edited