Check if writer is not used outside a change block

[pjasiun]

The model writer should be used only inside enqueueChange, change or post fixer block. It can be passed to the inner function, but should not be stored in a variable and used outside of the proper block. Fortunately, we should be able to check it easily.

Each writer method, when called, should check if this.model._currentWritter == this and throw exception if it is not true.

[oskarwrobel]

I'm pretty sure that all uses o writer are inside a block. There are a few places where writer is passed as a param but everything is wrapped by a block. I'm talking about src in all repos.

[pjasiun]

This is not about checking the current state. This about adding a check for the future to ensure that third-party plugins developers will not do something stupid.

[ma2ciek]

I'm assuming that all writer tests will need the model.change( writer => {} ) block since now?
Or we can use here beforeEach( () => { model._currentWriter = writer } ); to simplify Writer's tests a lot?

EDITED: Now I see, that this can't be done with the above hack, so all tests need to be rewritten and should use the enqueChange block.

[oskarwrobel]

I'm assuming that all writer tests will need the model.change( writer => {} ) block since now?

This is not necessary for tests. Writer tests should create Writer instance by new Writer( ... )

[oskarwrobel]

Ok, I understand now. Writer tests will throw otherwise.

[oskarwrobel]

Maybe it will be OK to do something as

writer = new Writer();
model._currentwriter = writer;

for writer tests.

[ma2ciek]

I've thought the same. But some writer methods calls in tests internally change or enqueueChanges, which clears the currentWriter property.

E.g. this test uses it. (I put the throw new Error() inside the enqueueChanges method to show that.

Writer
    insert()
      ✖ should move element from one parent to the other within the same document (documentA -> documentA)
        Chrome 63.0.3239 (Mac OS X 10.13.2)
      Error
          at Model.enqueueChange (webpack:///packages/ckeditor5-engine/src/model/model.js:155:0 <- build/.automated-tests/entry-point.js:18893:9)
          at clearAttributesStoredInElement (webpack:///packages/ckeditor5-engine/src/model/documentselection.js:741:0 <- build/.automated-tests/entry-point.js:16275:8)
          at DocumentSelection.listenTo (webpack:///packages/ckeditor5-engine/src/model/documentselection.js:100:0 <- build/.automated-tests/entry-point.js:15632:5)
          at Document.fire (webpack:///packages/ckeditor5-utils/src/emittermixin.js:269:0 <- build/.automated-tests/entry-point.js:4358:29)
          at Model.Document.listenTo (webpack:///packages/ckeditor5-engine/src/model/document.js:130:0 <- build/.automated-tests/entry-point.js:26571:10)
          at Model.fire (webpack:///packages/ckeditor5-utils/src/emittermixin.js:269:0 <- build/.automated-tests/entry-point.js:4358:29)
          at Model.(anonymous function) [as applyOperation] (webpack:///packages/ckeditor5-utils/src/observablemixin.js:337:0 <- build/.automated-tests/entry-point.js:27795:16)
          at Writer.insert (webpack:///packages/ckeditor5-engine/src/model/writer.js:182:0 <- build/.automated-tests/entry-point.js:14690:14)

I also think, that using model.enqueueChange( ( batch, writer ) => {} ) shows the proper way of using that class.

[pjasiun]

I think there are ways we could overwrite this check. For instance, the check could be done in a separate protected method which can be overwritten. However, in all cases it will be a hack and test will not check what they really should check. Especially more complicated tests could be unstable and hard to debug with such hacks.

So, I think it will be better to have these tests using change block with no hacks.

However, to make tests writing simpler they could use helpers like:

function createElement( name ) {
    model.change( writer => {
        writer.createElement( 'child' );
    }
}

[oskarwrobel]

For instance, the check could be done in a separate protected method which can be overwritten.

I was thinking about the same.

[ma2ciek]

I think there are ways we could overwrite this check. For instance, the check could be done in a separate protected method which can be overwritten. However, in all cases it will be a hack and test will not check what they really should check. Especially more complicated tests could be unstable and hard to debug with such hacks.
So, I think it will be better to have these tests using change block with no hacks.
However, to make tests writing simpler they could use helpers

I agree with this approach. But note, that there will be a lot of such methods and some of them will be a little bit longer. E.g. createElement will look actually like this:

function createElement( name, attributes ) {
    let element;

    model.change( writer => {
        element = writer.createElement( name, attributes );
    }

    return element;
}

And it will be possible only in tests, that don't check batches. Otherwise, we'd have to implement the second method using enqueueChanges.

[Reinmar]

Which makes me think that change() should return its callback's return value :D

function createElement( name, attributes ) {
    return model.change( writer => writer.createElement( name, attributes ) );
}

[ma2ciek]

And it will be possible only in tests, that don't check batches. Otherwise, we'd have to implement the second method using enqueueChanges.

Or maybe we could use enqueueChanges for each helper and create new batch before each test?

[pjasiun]

Which makes me think that change() should return its callback's return value :D

function createElement( name, attributes ) {
    return model.change( writer => writer.createElement( name, attributes ) );
}

It does return. You should write these methods this way.

[ma2ciek]

Which makes me think that change() should return its callback's return value :D

function createElement( name, attributes ) {
return model.change( writer => writer.createElement( name, attributes ) );
}

It does return. You should write these methods this way.

Oh, so I'll change them.