Skip to content

[FFM-1501] initial waitForInit implementation #21

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

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Conversation

enver-bisevac
Copy link
Contributor

This PR introduce waitFor init before any usage of public methods

@enver-bisevac enver-bisevac requested review from knagurski, davejohnston and a user October 14, 2021 10:38
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "ff-nodejs-server-sdk",
"version": "1.0.1",
"version": "1.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we introduced a breaking change by completely changing the API of the SDK to be asynchronous across the board, this really aught to be at 2.0.0 by now if we're sticking to SemVer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not breaking anything, it is just minor feature maybe it should be 1.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.

By changing the returns of all the variation function to be promises, everything is broken. Code written for v1.0.0 will not work with v1.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.

Also, this should only be v1.1.1 if it only fixes bugs in v1.1.0, if there are any breaking changes (e.g. changes in function signatures/return types), then it should be v2.0.0 and any new features or changes that do not break the existing signature should be v1.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@milos85vasic-harness and I decide to remove 1.0.0 and just go with 1.0.1 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a bug fix? It looks like a new feature, so at least v1.1.0. If we have no users of this SDK breaking between v1.0.0 and v1.0.1 is fine, but if we do and they update their dependencies, the change to use promises will break their app. We need to be religious about semver to avoid the support calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new thing is we introduce waitForInit so I think it is a requested feature from customer :), So that is the reason for 1.1.0 version

Copy link
Contributor

@knagurski knagurski left a comment

Choose a reason for hiding this comment

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

Some good work. Just a bit of food for thought and a question about our version numbering

@enver-bisevac enver-bisevac requested review from knagurski and a user October 14, 2021 11:58
@enver-bisevac enver-bisevac merged commit d8ad914 into main Oct 14, 2021
@enver-bisevac enver-bisevac deleted the FFM-1501 branch October 14, 2021 14:35
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.

2 participants