-
Notifications
You must be signed in to change notification settings - Fork 10
Align code to the new ckeditor5-cloudservices
package
#8
Conversation
tests/_utils/gettokenurl.js
Outdated
|
||
xhr.send( null ); | ||
} ); | ||
return `${ CLOUD_SERVICES_TOKEN_URL }?user.id=${ userId }`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newest CS should support anonymous users. It means that you don't need to set user.id
, and the whole file is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do something like this in every sample and snippet?
cloudServices: {
tokenUrl: 'https://j2sns7jmy0.execute-api.eu-central-1.amazonaws.com/prod/token'
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"every sample"
Is there more than one? :P
I would go with:
const CLOUD_SERVICES_TOKEN_URL = 'https://j2sns7jmy0.execute-api.eu-central-1.amazonaws.com/prod/token';
//...
cloudServices: {
tokenUrl: CLOUD_SERVICES_TOKEN_URL
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in the snippets in ckeditor5-image
: e.g. here: https://github.com/ckeditor/ckeditor5-image/blob/master/docs/_snippets/features/image.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ach, then I would: export CLOUD_SERVICES_TOKEN_URL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wait with changes for the correct token URI.
src/cloudservicesuploadadapter.js
Outdated
@@ -26,7 +27,7 @@ export default class CloudServicesUploadAdapter extends Plugin { | |||
* @inheritDoc | |||
*/ | |||
static get requires() { | |||
return [ FileRepository ]; | |||
return [ FileRepository, CloudServicesUploadAdapter._CloudServices ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use to mock plugins. I would mock CloudServices
class in CloudServices
plugin instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
package.json
Outdated
@@ -7,7 +7,8 @@ | |||
"ckeditor5-feature" | |||
], | |||
"dependencies": { | |||
"@ckeditor/ckeditor-cloudservices-core": "^0.1.0", | |||
"@ckeditor/ckeditor-cloudservices-core": "^0.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be changed to "^0.2.0".
package.json
Outdated
@@ -7,7 +7,8 @@ | |||
"ckeditor5-feature" | |||
], | |||
"dependencies": { | |||
"@ckeditor/ckeditor-cloudservices-core": "^0.1.0", | |||
"@ckeditor/ckeditor-cloudservices-core": "^0.2.0", | |||
"@ckeditor/ckeditor5-cloudservices": "^1.0.0-alpha.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that until we will release it on npm, it should be as:
"@ckeditor/ckeditor5-cloudeservices": "ckeditor/ckeditor5-cloudservices"
Otherwise there will be errors.
tests/cloudservicesuploadadapter.js
Outdated
uploadUrl: 'http://upload.mock.url/' | ||
} | ||
} ) | ||
.then( editor => { | ||
expect( UploadGatewayMock.lastToken ).to.equal( 'abc' ); | ||
expect( UploadGatewayMock.lastToken.value ).to.equal( 'token' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not check internals of the token class here. I would remove this assertion since this test is not about it.
Other: Aligned code to the new
ckeditor5-cloudservices
package. Closes ckeditor/ckeditor5#2558.