Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix(types): type declaration for AuthenticateOptions.scope #305

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
jankapunkt merged 1 commit into node-oauth:master from Lordfirespeed:patch-1
Jul 24, 2024

Conversation

@Lordfirespeed
Copy link
Contributor

@Lordfirespeed Lordfirespeed commented Jul 24, 2024
edited
Loading

Resolves #306

Copy link
Contributor

dhensby commented Jul 24, 2024

This change means the types reflect the real usage, so definitely should merge - but this touches on our discussion some time ago where I thought the conclusion was that we should be using scopes as arrays of strings for internal purposes but space delimited strings for the external interfaces. I'd have expected this to accept an array and not a string.

I'm not suggesting we change the implementation right now, but something to think about and potentially change for the next (major) release.

Copy link
Member

I've been on vacation so I did not completely read the discussion, however I agree doing a breaking change with a major release.

@jankapunkt jankapunkt merged commit 3fa9598 into node-oauth:master Jul 24, 2024
Copy link
Contributor Author

Lordfirespeed commented Jul 24, 2024
edited
Loading

Guys, the breaking change actually snuck into v5.1:
fcb567b changed the accepted type from string[] (as in v5) to string.
So this should probably be reverted.

Copy link
Contributor Author

Copy link
Member

I remember there was a discussion on standard compliance which led to this change @dhensby correct?

dhensby reacted with thumbs up emoji

Copy link
Contributor

dhensby commented Jul 24, 2024

The changes made in #267 we to bring the library back in-line with OAuth specification and thus compliance. The change was a bug fix because it's a stated aim of the library to be standard compliant.

Whether that meant that some internal parts of the system that should have remained as arrays of strings were changed to just strings at the same time, may be the case, but really it's completely academic - 5.0.0 was a bad release based on not being standards compliant and 5.1.0 has been out for over 4 months with no complaints, to revert it to accept an array and not a string now would just be a dogmatic fixation on SemVer for no gain and just a lot of disruption.

We could look at accepting both space delimited strings and array of strings so we can technically adhere to semver, but I wouldn't be making a fuss about that myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhensby dhensby dhensby approved these changes

@jankapunkt jankapunkt jankapunkt approved these changes

@jorenvandeweyer jorenvandeweyer Awaiting requested review from jorenvandeweyer

@HappyZombies HappyZombies Awaiting requested review from HappyZombies

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AuthenticateOptions.scope type is declared incorrectly

AltStyle によって変換されたページ (->オリジナル) /