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

Created release script that updates all ckeditor5 dependencies to the latest version. #737

Merged
merged 29 commits into from Jan 7, 2022

Conversation

przemyslaw-zan
Copy link
Member

@przemyslaw-zan przemyslaw-zan commented Dec 14, 2021

Feature (env): Created a script for updating versions of ckeditor5 and @ckeditor/ckeditor5-* dependencies to a specified version.

@coveralls
Copy link

coveralls commented Dec 14, 2021

Coverage Status

Coverage increased (+0.7%) to 87.554% when pulling 6aa65a5 on ci/1123 into c49ca41 on master.

@psmyrek
Copy link
Contributor

psmyrek commented Dec 16, 2021

Please also add test in packages/ckeditor5-dev-env/tests/index.js for the newly-added task, so the coverage would not be decreased.

@ckeditor ckeditor deleted a comment from przemyslaw-zan Dec 17, 2021
@przemyslaw-zan
Copy link
Member Author

*/
function formatDiff( diff ) {
const formattedDiff = [];
const regex = /(?<=":) (?=")/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this regexp is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to split these lines and merge them back together in order to pretty print the diff. Indentations are also kept in these lines, so there are more than one space. I suppose i could split using last space of a string instead, but this seems safer.

    "@ckeditor/ckeditor5-core": "^31.1.0",

Copy link
Contributor

Choose a reason for hiding this comment

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

What if, for whatever reason, the script changes something else in the package.json file, other than the CKEditor 5 dependency versions? Now, only changes to the dependency versions are displayed in red and green colors, but other changes that could occur in the file are not highlighted in the console.

The --dry-run flag should display all the changes that the script has made, to be sure that running it without this flag will not surprise us.

If the dependency version has changed, it can be highlighted as it is now (only the version, not the full line). But when something else has changed that is not expected, let the entire changed line be highlighted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, i've actually taken this into account, but then i forgot to color the rest of changes, so good thing You pointed that out 😅

Single removals and additions are simple enough, they are just colored lines. But how do we want to show replacements?

a) Replacement on the same line

b) Replacement on the same line, but trim the whitespaces off the addition

c) Replacement on the line below 

I'd suppose option b) is preferable, so this is what I'll do for now.

*/
function formatDiff( diff ) {
const formattedDiff = [];
const regex = /(?<=":) (?=")/;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if, for whatever reason, the script changes something else in the package.json file, other than the CKEditor 5 dependency versions? Now, only changes to the dependency versions are displayed in red and green colors, but other changes that could occur in the file are not highlighted in the console.

The --dry-run flag should display all the changes that the script has made, to be sure that running it without this flag will not surprise us.

If the dependency version has changed, it can be highlighted as it is now (only the version, not the full line). But when something else has changed that is not expected, let the entire changed line be highlighted.

@przemyslaw-zan
Copy link
Member Author

I've encountered some problems while trying to write more tests as requested. The function is currently set up as a synchronous one. However, when --dry-run is used, it has asynchronous elements, as it waits for the user input. This did not cause problems untill i've tried to write some tests that would cover said option. This causes problem of multiple scripts awaiting anwser at the same time. I've tried converting it to asynchronous function by wraping it's content into Promise and using return resolve() instead of process.exit(), but this does not end the script, and it still awaits anwser, even if normally it shouldnt, as there are no changes. This makes me think that somehow, return resolve() does not kill readline. Moreover, I've failed to come up with a way to simulate keypreses to test it, and only found dead-end unawsered question. What should we do with this? I've found readline-sync package, and succesfylly replaced asynchronous readline (with just small inconvinience of the script no longer taking enter or arrow keys, etc. as an input, only a-z, 0-9 and other characters). But even after that, testing is a problem. I can't simulate any keypress, if the synchronous function is still executing, waiting for input. But even in asynchronous form, how would we give input to the function, before .then() can be called? I suppose setTimeout() could be used, but that feels terribly hacky...

@psmyrek
Copy link
Contributor

psmyrek commented Dec 31, 2021

I've encountered some problems while trying to write more tests as requested. The function is currently set up as a synchronous one. However, when --dry-run is used, it has asynchronous elements, as it waits for the user input. This did not cause problems untill i've tried to write some tests that would cover said option. This causes problem of multiple scripts awaiting anwser at the same time. I've tried converting it to asynchronous function by wraping it's content into Promise and using return resolve() instead of process.exit(), but this does not end the script, and it still awaits anwser, even if normally it shouldnt, as there are no changes. This makes me think that somehow, return resolve() does not kill readline. Moreover, I've failed to come up with a way to simulate keypreses to test it, and only found dead-end unawsered question. What should we do with this? I've found readline-sync package, and succesfylly replaced asynchronous readline (with just small inconvinience of the script no longer taking enter or arrow keys, etc. as an input, only a-z, 0-9 and other characters). But even after that, testing is a problem. I can't simulate any keypress, if the synchronous function is still executing, waiting for input. But even in asynchronous form, how would we give input to the function, before .then() can be called? I suppose setTimeout() could be used, but that feels terribly hacky...

I don't know if I understood what you mean exactly, but I think it would be sufficient to just mock the readline.emitKeypressEvents(), process.exit() and the process.stdin object with on() and setRawMode() functions. Then, your mocked process.stdin.on() function will be called, so you can register a callback for the provided event name. Then, you can simulate a keystroke by trying to call a callback function registered to the keypress event, with appropriate arguments that are expected in the source file.

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants