pubsub: default autoCreate:true #685
Conversation
googlebot
added
the
cla: yes
|
@ryanseys sorry to disturb!, but curious of your thoughts on this one, mostly in terms of if privatizing createTopic and defaulting autoCreate to true sounds like a good plan? Thanks! |
|
Yep. I'll look at it today. Hang tight :) sorry for the delay.
|
|
No rush at all! If this doesn't make it in by tomorrow, we can just pack it up in 0.17.0. |
|
Doesn't sound like a bad idea unless someone wants to create a lot of topics all the time... e.g. what if I'm making a public chatroom app and want to create a topic for each room being created? In this case, every request to use the topic will result in an extraneous request to attempt to use a non-existent topic first and could just not be the most efficient. Have you considered leaving createTopic public, making autoCreate true by default and updating the createTopic docs to describe that the first time you try to use a topic it will auto create one by default anyway so you don't need to use the method except in certain cases? This would also allow the method to be more smoothly deprecated. |
|
|
||
| // Reference an existing topic. | ||
| var topic = pubsub.topic('my-existing-topic'); | ||
| var topic = pubsub.topic('my-topic'); | ||
|
|
||
| // Publish a message to the topic. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Code looks good to me. Not against the idea, just definitely should consider all alternatives such as the one I mentioned above. |
|
Thanks, sir! I'm in favor of keeping the method public as well, but only for the reason that users would actually use it. I don't mind harshly deprecating it since we're still pre-stable. |
|
It's definitely not like we're getting rid of, or not supporting, the ability to create topics (that would be silly) so I don't see the need to hide it behind some private flag. |
|
If @jgeewax agrees we should keep it public, I'll make the changes to the PR, fix the docs, and we can be on our mergy way. |
|
Yea I can see the need to have Cool? |
|
Yeah. First post updated with todos. |
stephenplusplus
force-pushed
the
spp--pubsub-privatize-create-topic
branch
from
c43f35f
to
cc38f88
Compare
stephenplusplus
changed the title
stephenplusplus
force-pushed
the
spp--pubsub-privatize-create-topic
branch
3 times, most recently
from
6e33489
to
e5b52d5
Compare
|
To Dos have been To Done. |
|
|
||
| // Publish a message to the topic. | ||
| // The topic will be created if it doesn't exist. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
stephenplusplus
force-pushed
the
spp--pubsub-privatize-create-topic
branch
from
e5b52d5
to
2b9e1f9
Compare
| @@ -160,8 +160,16 @@ PubSub.prototype.getTopics = function(query, callback) { | |||
| * | |||
| * @example | |||
| * pubsub.createTopic('my-new-topic', function(err, topic, apiResponse) { | |||
| * topic.publish('New message!', function(err) {}); | |||
| * topic.publish({ | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Didn't spend too much time looking through but this looks pretty good to me 👍 |
…eate-topic pubsub: default autoCreate:true
|
If anyone has doc improvements, we can iterate on them as we always do :) |
Fixes #669
This privatizescreateTopicby removing it from the docs and renaming it tocreateTopic_. It is still used internally withautoCreate: true, which is nowtrueby default.The other changes in this PR include doc/test changes to suit the new behavior, and refactoring how the
autoCreatebehavior is implemented (+ tests).To Dos
createTopiccreateTopicdocs statingtopic()is much easier, and likely what the user intends