Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added "@ckeditor/ckeditor-cloud-services-core" package to this repo #1467

Merged
merged 12 commits into from
Sep 9, 2019

Conversation

pomek
Copy link
Member

@pomek pomek commented Jan 16, 2019

Suggested merge commit message (convention)

Internal: Added @ckeditor/ckeditor-cloud-services-core package to this repository. Closes #924.


Additional information

Requires:

@pomek pomek requested a review from Reinmar January 16, 2019 13:36
@Reinmar Reinmar self-assigned this Feb 15, 2019
mgit.json Outdated
@@ -42,5 +46,8 @@
"@ckeditor/ckeditor5-upload": "ckeditor/ckeditor5-upload",
"@ckeditor/ckeditor5-utils": "ckeditor/ckeditor5-utils",
"@ckeditor/ckeditor5-widget": "ckeditor/ckeditor5-widget"
},
"overrideDirectoryNames": {
"@ckeditor/ckeditor-cloud-services-core": "ckeditor5-cloud-services-core"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't lime the fact that we're renaming package which name starts with ckeditor- to ckeditor5-. In my original proposal in cksource/mrgit#72 I proposed to have a different directory structure, where all the prefixes (ckeditor5-*) are stripped. So, according to my comment, this setting here should be:

 "overrideDirectoryNames": {
      "@ckeditor/ckeditor5-cloud-services-core": "cloud-services-core"
  }

So, the actual directory name should be cloud-services-core.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/ckeditor/ckeditor5/pull/1467/files#r303510548. My proposal was to change all the names to get rid of any prefixes there. We can discuss if that's a good change, but this PR for now implements something else, hence R-.

@Reinmar
Copy link
Member

Reinmar commented Jul 15, 2019

@Reinmar
Copy link
Member

Reinmar commented Sep 2, 2019

I asked the team for a feedback on this and they see this a bit differently than I did:

image

So, we need to change our approach here a bit, unfortunately.

@Reinmar
Copy link
Member

Reinmar commented Sep 2, 2019

@pomek
Copy link
Member Author

pomek commented Sep 4, 2019

Docs

I fixed as much as I could but I have no idea what to do with the errors below:

JSDoc started...
Incorrect param type: Blob
        at /Users/pomek/Projects/ckeditor/ckeditor5/packages/ckeditor-cloud-services-core/src/uploadgateway/fileuploader.js (line 29)

Incorrect param type: Blob
        at /Users/pomek/Projects/ckeditor/ckeditor5/packages/ckeditor-cloud-services-core/src/uploadgateway/uploadgateway.js (line 74)

Incorrect link: Token
        at /Users/pomek/Projects/ckeditor/ckeditor5/packages/ckeditor-cloud-services-core/src/token/token.js (line 150)

Invalid return type: Blob.                                                                                                                                                                                                                            at /Users/pomek/Projects/ckeditor/ckeditor5/packages/ckeditor-cloud-services-core/src/uploadgateway/fileuploader.js (line 248)


Validation Summary:
 * Validator found 4 errors.

Good news, #1978 will be resolved after adding the package to our repos.

@pomek
Copy link
Member Author

pomek commented Sep 5, 2019

Release tools

image

image

@pomek
Copy link
Member Author

pomek commented Sep 5, 2019

Content styles works. I don't remember what I should check in the "Theme" topic but if anything won't work, MT and Content styles scripts would print any error.

@pomek
Copy link
Member Author

pomek commented Sep 5, 2019

After merging PRs: ckeditor/ckeditor5-dev#554 and ckeditor/ckeditor-cloud-services-core#27, CI should work.

@pomek
Copy link
Member Author

pomek commented Sep 5, 2019

Error codes from the package are visible in the error table:

image

@pomek
Copy link
Member Author

pomek commented Sep 5, 2019

I found this:

image

@pomek
Copy link
Member Author

pomek commented Sep 5, 2019

Fixed.

image

mrgit.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Reinmar
Copy link
Member

Reinmar commented Sep 9, 2019

(t/924 b7bb733 M1) p@m /workspace/ckeditor5> yarn test --files=cloud-services-core -c
yarn run v1.17.3
$ node --max_old_space_size=4096 node_modules/@ckeditor/ckeditor5-dev-tests/bin/test.js --files=cloud-services-core -c
TypeError: glob.match is not a function
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
(t/924 b7bb733 M1) p@m /workspace/ckeditor5>

@pomek
Copy link
Member Author

pomek commented Sep 9, 2019

@Reinmar, the error has been fixed (ckeditor/ckeditor5-dev@18feef7).

mrgit.json Outdated Show resolved Hide resolved
@Reinmar Reinmar merged commit c21c2ed into master Sep 9, 2019
@Reinmar Reinmar deleted the t/924 branch September 9, 2019 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add @ckeditor/ckeditor-cloud-services-core to this repo
2 participants