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(UserSession): switch "duration" to "expiration" in IOAuth2Options #845

Closed
wants to merge 1 commit into from
Closed

fix(UserSession): switch "duration" to "expiration" in IOAuth2Options #845

wants to merge 1 commit into from

Conversation

gavinr
Copy link
Contributor

@gavinr gavinr commented May 18, 2021

AFFECTS PACKAGES:
@esri/arcgis-rest-auth

BREAKING CHANGE:
IOAuth2Options: "duration" is removed. Please use "expiration"

ISSUES CLOSED: #843

@tomwayson is there a better way to deprecate?

AFFECTS PACKAGES:
@esri/arcgis-rest-auth

BREAKING CHANGE:
IOAuth2Options: "duration" is removed. Please use "expiration"

ISSUES CLOSED: #843
@gavinr gavinr requested review from tomwayson and hogpilot May 18, 2021 04:23
@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #845 (8d007aa) into master (3f43679) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #845   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          133       133           
  Lines         2352      2352           
  Branches       415       415           
=========================================
  Hits          2352      2352           
Impacted Files Coverage Δ
packages/arcgis-rest-auth/src/UserSession.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f43679...8d007aa. Read the comment docs.

Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

thanks @gavinr, but I think we should either hold this until the next release, or change it to deprecate duration instead of remove it.

*/
duration?: number;
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking we'd have both until the next breaking change so we would not have to do a breaking change for this release. We'd leave duration, but change the comments to indicate that it is deprecated and to use expiration instead. Then in the code below we'd use expiration || duration until the next major release.

Copy link
Contributor Author

@gavinr gavinr May 18, 2021

Choose a reason for hiding this comment

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

got it. That's what I initlally was doing, using the @deprecated tag, but then the build was erroring, saying it should not be used. I think I was probably just not doing it correctly. I'll bring that code back into this PR then re-ping you. Thanks!

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.

add an expiration to and deprecate duration from IOAuth2Options
2 participants