Skip to content

Improving generateId method#535

Merged
darrachequesne merged 9 commits intosocketio:masterfrom
efkan:master
Oct 20, 2017
Merged

Improving generateId method#535
darrachequesne merged 9 commits intosocketio:masterfrom
efkan:master

Conversation

@efkan
Copy link
Copy Markdown
Contributor

@efkan efkan commented Oct 16, 2017

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

If overwritten and assigned generateId function needs some extra operation like Redis query, id value passed as undefined because Javascript runs asynchronously.

New behaviour

Adding a callback to generateId method provides the program flow to continue after a new generated id is provided.

Other information (e.g. related issues)

Also README file has been changed according to the updates above.

efkan added 3 commits October 16, 2017 13:55
@efkan
Changing generateId method
ce2516a 
Adding a callback to generateId method provides the program flow to continue after a new generated id is provided.
@efkan
Update README.md
b8d8c8a
@efkan
Changing generateId method
88edb13 
Adding callback to generateId method
darrachequesne
darrachequesne reviewed Oct 17, 2017
View reviewed changes
Comment thread lib/server.js Outdated
Server.prototype.generateId = function (req) {
return base64id.generateId();
Server.prototype.generateId = function (req, callback) {
callback(base64id.generateId());
Copy link
Copy Markdown
Member

@darrachequesne darrachequesne Oct 17, 2017

Choose a reason for hiding this comment

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

How about callback(null, base64id.generateId());, so that errors can also be handled (see #518)?

Copy link
Copy Markdown
Contributor Author

@efkan efkan Oct 17, 2017

Choose a reason for hiding this comment

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

You're right! I'll update the code.

@darrachequesne darrachequesne merged commit 2abb217 into socketio:master Oct 20, 2017
@darrachequesne
Copy link
Copy Markdown
Member

darrachequesne commented Oct 20, 2017

@efkan merged, thanks!

@darrachequesne darrachequesne mentioned this pull request Oct 20, 2017
Closed
5 tasks
@darrachequesne darrachequesne added this to the 4.0.0 milestone Oct 20, 2017
darrachequesne added a commit that referenced this pull request Feb 27, 2018
@darrachequesne
[revert] Make generateId method async (#535)
c624751 
That is a breaking change, which mandates a major bump.
@efkan
Copy link
Copy Markdown
Contributor Author

efkan commented Mar 7, 2018

Hello @darrachequesne ,

I've seen that this PR had to be reverted.

[revert] Make generateId method async
That is a breaking change, which mandates a major bump.

I could not find any issue about it.
I was wondering that what is the breaking change and is there anything I can do to help on this?

@efkan efkan mentioned this pull request Mar 7, 2018
Closed
@darrachequesne
Copy link
Copy Markdown
Member

darrachequesne commented Mar 7, 2018

@efkan I'm currently working on releasing a new version of socket.io, but yes both of your propositions (async generateId and #538) should be included in version [email protected], sorry for the delay.

@efkan
Copy link
Copy Markdown
Contributor Author

efkan commented Mar 7, 2018 •
edited
Loading

@darrachequesne , Do you want me to apply my code again on a new PR?

@darrachequesne
Copy link
Copy Markdown
Member

darrachequesne commented Mar 9, 2018

@efkan I'll cherry-pick it.

@darrachequesne darrachequesne mentioned this pull request May 17, 2018
Closed
9 tasks
@ghost
Copy link
Copy Markdown

ghost commented Jul 27, 2018

Can we make this support Promises (async functions) as well as callbacks?

darrachequesne added a commit that referenced this pull request Jan 12, 2020
@darrachequesne
feat: generateId method can now return a Promise
d6c7128 
Related:

- #538
- #535
darrachequesne added a commit that referenced this pull request Jan 14, 2020
@darrachequesne
feat: generateId method can now return a Promise
f3c291f 
Related:

- #538
- #535
@snyk-bot snyk-bot mentioned this pull request May 27, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darrachequesne darrachequesne darrachequesne left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@efkan @darrachequesne