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

2.0.0 (aka 1.0.0) #238

Open
wants to merge 61 commits into
base: master
Choose a base branch
from
Open

2.0.0 (aka 1.0.0) #238

wants to merge 61 commits into from

Conversation

hypesystem
Copy link
Collaborator

@hypesystem hypesystem commented May 29, 2016

should not be merged

This is a long-running branch that will exist while we are figuring out the exact interface of version 1.0.0 of this project.

New changes to the v1 branch should be made as separate PRs merging into v1.

The current latest alpha of 1.0.0 is 1.0.0-alpha.1. It is not recommended in production. Use instead 0.14.2, which lives on the master branch of this repository.

Preliminary TODO for 1.0.0:

  • Remove deprecated things
    • Constants
    • Result, MulticastResult
    • Message#addDataWith...
  • Remove Sender#sendNoRetry (replace with a cleverer Sender#send)
  • Simpler recipient argument: closer to the server side interface.
  • Remove Message (replace with plain JSON)
  • Only have options for the Sender constructor (retries and backoff are usually used very statically).
  • Better error reporting from Sender#send
  • Better, more actionable response from Sender#send (easier to tell which registration ids should be deleted, re-registered, etc --- check for all potential errors what should be done)
  • Support sending a notification to more than 1000 recipients at a time
  • Proper validation of registration ids. (Can we actually know if they are valid before they are sent?)
  • thoroughly test support for the to field
  • thoroughly test support for the registration_ids field
  • Update dependencies
  • Update min. node version

Comments, feedback, etc. on this process are very welcome 😄

@eladnava
Copy link
Collaborator

eladnava commented May 30, 2016

@hypesystem what do you say about removing lib/sender.js and moving all of its contents to the index.js file in the root project directory?

Seems kind of unnecessary to require 2 empty files on the way as it is today:

require('node-gcm') -> index.js -> lib/node-gcm.js -> lib/sender.js

@hypesystem
Copy link
Collaborator Author

@eladnava I think we are probably going to split the Sender file at some point, so it will make more sense. You are right: right now it is very silly. I would rather wait and see exactly what happens with the Sender, before deciding on the file hierachy 😄

@eladnava
Copy link
Collaborator

@hypesystem Isn't that 'some point' now since we are basically cleaning everything we think should be cleaned up? Or would you rather wait till the end of everything else?

@hypesystem
Copy link
Collaborator Author

@eladnava haha yeah, I'm not saying "let's wait for a long time" --- just until we are a bit further with the new v1 interface 😄

We have eliminated a lot of abstractions, and plan to add some more functionality to the Sender -- so it will grow quite big, and then it should be split into smaller files. After that, we can figure out how our index.js should look 😄

@hypesystem hypesystem mentioned this pull request May 30, 2016
This required some major changes to how the tests are structured, as I didnt want to be listening on what used to be "sendNoRetry".
requestStub is now a sinon spy.
it is reset before EVERY test (woops, before we only cleared state before each test CATEGORY)
and how we expect some things has changed as a result of these structural changes
They used the registration_ids key, which we will no longer support.
They passed because state was NOT reset before every test (fixed this in earlier commit)
@hypesystem hypesystem mentioned this pull request Jun 9, 2016
@hypesystem
Copy link
Collaborator Author

Published 1.0.0-alpha.1 😄

@eladnava
Copy link
Collaborator

eladnava commented Jun 9, 2016

Awesome! :octocat: 😄

@hypesystem
Copy link
Collaborator Author

How will we handle users warning to send to topics fitting a condition?

@eladnava
Copy link
Collaborator

@hypesystem How do you currently detect a topic versus a registration token passed in via the second argument?

@hypesystem
Copy link
Collaborator Author

@eladnava Currently we do not have to. Topics are passed to the to field, just like a single registration token should be. This was done to move closer to the actual HTTP interface, where that seemed to be the distinction. With conditions, this changes slightly. I will try to come up with some idea for a solution when I am back from summer holidays.

@eladnava
Copy link
Collaborator

eladnava commented Jul 24, 2016

We could instead change the interface to allow passing in an object (but the same exact object later passed into the request, e.g. to, registration_ids, or condition). If an array is passed in, treat it like registration_ids, and if a string, set it as the to.

@hypesystem
Copy link
Collaborator Author

That would definitely be one option 😄 it's quite verbose, but I guess that the simple string and array options will be the common cases, and they will still be supported... 👍

@eladnava eladnava changed the title 1.0.0 2.0.0 May 15, 2018
@eladnava eladnava changed the title 2.0.0 2.0.0 (aka 1.0.0) May 15, 2018
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.

3 participants