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

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

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

Lordfirespeed
Copy link
Contributor

@Lordfirespeed Lordfirespeed commented Jul 24, 2024

Resolves #306

@dhensby
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.

@jankapunkt
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
9 checks passed
@Lordfirespeed
Copy link
Contributor Author

Lordfirespeed commented Jul 24, 2024

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.

@Lordfirespeed
Copy link
Contributor Author

@dhensby @jankapunkt

@jankapunkt
Copy link
Member

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

@dhensby
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AuthenticateOptions.scope type is declared incorrectly
3 participants