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

chore(Observable): deprecate create method (#3982) #4080

Merged
merged 1 commit into from
Nov 27, 2018

Conversation

dkosasih
Copy link
Contributor

@dkosasih dkosasih commented Sep 1, 2018

Description: deprecate all the create method, use new instead

Related issue (if exists): #3982

@dkosasih
Copy link
Contributor Author

dkosasih commented Sep 1, 2018

@cartant I am not sure if the commit message type should be chore - do you want this changes to make it to changelog?

@dkosasih
Copy link
Contributor Author

dkosasih commented Sep 3, 2018

Hi @cartant
I have been trying to figure out why is this failing while the only thing I do it to mark creation method as deprecate. Could you please advise?

@cartant
Copy link
Collaborator

cartant commented Sep 3, 2018

The error isn't related to your change; it's a problem with dtslint. See #4079

However, I'd suggest that you leave this as-is for the moment and don't apply the workaround that's mentioned in that issue.

@dkosasih
Copy link
Contributor Author

dkosasih commented Sep 3, 2018

I was trying to figure out why is this failing. I tried dtslint on my local machine and it was fine.
Thanks for letting me know. Happy Monday.

@@ -39,7 +39,9 @@ export class Subject<T> extends Observable<T> implements SubscriptionLike {
super();
}

/**@nocollapse */
/**@nocollapse
* @deprecated use new Subject() instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. I'm not sure about this deprecation message, as this create method does things that cannot be done with the Subject constructor alone. Your thoughts, @benlesh ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not very sure about this one too because the constructor signature is different. Observable one is more straight forward.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine. The only thing that create did that new Observable couldn't was allow apply and call, which people can get from a simple factory function.

@@ -39,7 +39,9 @@ export class Subject<T> extends Observable<T> implements SubscriptionLike {
super();
}

/**@nocollapse */
/**@nocollapse
* @deprecated use new Subject() instead
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine. The only thing that create did that new Observable couldn't was allow apply and call, which people can get from a simple factory function.

@benlesh
Copy link
Member

benlesh commented Sep 7, 2018

@dkosasih can you rebase, please?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 96.798% when pulling 7a52531 on dkosasih:deprecate_create into c1c66d2 on ReactiveX:master.

@dkosasih
Copy link
Contributor Author

dkosasih commented Sep 8, 2018

Done 😊

@benlesh benlesh merged commit 660133d into ReactiveX:master Nov 27, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018
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.

4 participants