Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

Conversation

patricksmms
Copy link
Contributor

@patricksmms patricksmms commented Aug 27, 2020

Adding method that is more convenient to use, when not running on a web worker or as a service, as it blocks the main-thread regardless.

@patricksmms patricksmms requested a review from kylef August 27, 2020 12:45
@patricksmms patricksmms force-pushed the patricksmms/add-fury-serialize-sync branch from e157e7e to a169684 Compare August 27, 2020 19:26
@patricksmms patricksmms force-pushed the patricksmms/add-fury-serialize-sync branch from b0754f6 to fc81a6d Compare August 31, 2020 11:34
@patricksmms patricksmms force-pushed the patricksmms/add-fury-serialize-sync branch from fc81a6d to 2b80fbe Compare August 31, 2020 11:40
Copy link
Member

@kylef kylef left a comment

Choose a reason for hiding this comment

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

Looks good to me, I've left some minor comments.


/**
* Serialize an API Description into the given output format.
* Serialize asynchronously an API Description into the given output format.
Copy link
Member

Choose a reason for hiding this comment

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

Grammatically, I think it would sound better to change the order here. The adverb (asynchronously) describes the word serialise. I think the rules of time adverbs would apply here so either "asynchronously serialise" or "serialise X asynchronously" instead of "serialise asynchronously X"

Suggested change
* Serialize asynchronously an API Description into the given output format.
* Asynchronously serialize an API Description into the given output format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 20ee86c

});
});

it(`serializes synchronously ${path.basename(file)}`, (done) => {
Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't an async API, perhaps it would be clearer to omit done. I think the test would be simpler without it since you won't need to wrap the blocks in try/catch as throwing from the test would trigger test failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in cd072b1

@@ -1,5 +1,11 @@
# API Elements: CLI Changelog

## 0.10.3 (2019-08-31)
Copy link
Member

Choose a reason for hiding this comment

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

No strong opinions here, although we don't necessarily need to release the CLI as these changes have no impact on it. The new version of the apib serialize adapter is also compatible with the existing version "@apielements/apib-serializer": "^0.16.1", so installing CLI would get updated apib-serializer regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I saw similar releases on the past, that's why I did it.

Copy link
Member

Choose a reason for hiding this comment

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

The past ones are usually a slightly different case where the version range didn't suffice (or core changed). Such as minor releases which don't fit into the patch range (0.17 for example won't fit into that range, and might trigger CLI release).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case I'll drop this release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped in e0d00c3

@patricksmms patricksmms merged commit e2a8c75 into master Sep 2, 2020
@patricksmms patricksmms changed the title feat(core): add serializeSync method feat(project): add serializeSync method Sep 2, 2020
@patricksmms patricksmms deleted the patricksmms/add-fury-serialize-sync branch September 2, 2020 11:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants