Pass required params to Session constructor. Allow `undefined` in AuthScopes by thecodepixi · Pull Request #169 · Shopify/shopify-api-js

thecodepixi · 2021-04-28T15:11:06Z

WHY are these changes introduced?

The parameters required by Session were previously not being passed to the constructor, and we were instantiating instances of Session with just their id. This caused some issues due to potentially missing required params.

An issue with scopes potentially being undefined and causing errors in the AuthScopes class is also fixed in this PR. This initially seemed like a Session issue but was able to be resolved in the AuthScope class itself by allowing undefined scopes in the constructor, which then translates to an empty array of scopes.

The change to AuthScopes should potentially fix #159 and similar issues.

WHAT is this pull request doing?

Moves all required Session params into the constructor.
- this is a potentially breaking change, depending on how directly users interact with Session
Updates all testing where we call new Session to pass the required params.
Updates AuthScopes to allow for undefined in the constructor, and then check for the presence of scope.

important to note that a large portion of this PR is updates to test files

Type of change

Patch: Bug (non-breaking change which fixes an issue)
Minor: New feature (non-breaking change which adds functionality)
Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

I have added a changelog entry, prefixed by the type of change noted above
I have added/updated tests for this change
I have documented new APIs/updated the documentation for modified APIs (for public APIs)

paulomarg

Looks great, just had a couple of nits!

paulomarg · 2021-04-29T13:01:50Z

    if (typeof scopes === 'string') {
      scopesArray = scopes.split(new RegExp(`${AuthScopes.SCOPE_DELIMITER}\\s*`));
-    } else {
+    } else if (scopes) {


Should we be extra sure that we only process accepted types?

Suggested change

} else if (scopes) {

} else if (scopes !== undefined) {

That's a good point. Should there be an exception/error at that point if scopes is not a string, array or undefined?

Technically, typescript would fail in that case, but javascript wouldn't. Then again, we don't check for that sort of thing in a lot of other places (and is that an excuse for not doing it here? 😅 ).

Yeah I'm unsure what the right move is here 😅

thecodepixi · 2021-04-29T16:06:13Z

This needs a good rebase and probably a bit of refactoring since we've merged #153. I'll get that taken care of so this should be in a good place soon.

paulomarg · 2021-04-29T19:12:27Z

 ## [1.2.1] - 2021-03-26
 ### Added
 - Added `April21` to `ApiVersion` [#149](https://github.com/Shopify/shopify-node-api/pull/149)
+- Sessions no longer default to `false` for `isOnline` [#169](https://github.com/Shopify/shopify-node-api/pull/169)


I'm a little confused - is this part of v1.2.1?

Also, I think this might be a breaking change, right? It will essentially break any apps that build sessions (which might be the case for custom session storages made prior to the change to allow objects).
Update: I actually don't think it's breaking! It's OK to not get those values in the constructor (if we make the params optional), and since the properties are all public, they can continue to be set as they were.

Ope, sorry. My changelog got all weird after I rebased. Will fix.

Also yes, this would be breaking. I don't think these values can be optional? They need to be there for other parts of the code that handle the Session to work, right?

Yes, but if you call the constructor and then set the fields, that's OK too, as long as the right fields continue to be set - i.e. apps that did it 'the old way' would continue to work if the constructor doesn't break because of missing params.

sagar-ranglani · 2021-09-17T15:08:07Z

Any updates on this one?

mariomx3 · 2021-10-12T15:56:29Z

Any update or a workaround for now?

hgezim · 2021-10-13T05:49:27Z

@thecodepixi Any chance you can resolve those conflicts?

mariomx3 · 2021-10-15T17:08:25Z

Will this be merged in soon? Really critical

paulomarg · 2021-10-15T19:00:30Z

Hi folks! Sorry for the delay on this one.

We're currently working on a few breaking changes that are going to make up v2 of the library. Since this is also a breaking change (because of the isOnline default), we'll make sure to include it in the new release.

Stay tuned, and thanks for your patience!

…tructor

thecodepixi force-pushed the thecodepixi/move-required-args-to-session-constructor branch 3 times, most recently from 679dd92 to 2444034 Compare April 28, 2021 21:09

thecodepixi changed the title ~~DRAFT Moves required Session args to constructor~~ Pass required params to Session constructor. Allow undefined in AuthScopes Apr 28, 2021

thecodepixi marked this pull request as ready for review April 28, 2021 21:22

thecodepixi requested review from mkevinosullivan and paulomarg April 28, 2021 21:22

paulomarg reviewed Apr 29, 2021

View reviewed changes

carmelal mentioned this pull request Apr 29, 2021

[WIP] Ensure scopes defined in scopes.equals() #160

Closed

4 tasks

thecodepixi added 2 commits April 29, 2021 12:09

Pass required args to Session constructor. Fix undefined scopes.

cff6350

Rebase main. Add necessary changes to Session and AuthScope.

8bf068e

thecodepixi force-pushed the thecodepixi/move-required-args-to-session-constructor branch from 2444034 to 8bf068e Compare April 29, 2021 18:59

thecodepixi requested a review from a team as a code owner April 29, 2021 18:59

thecodepixi force-pushed the thecodepixi/move-required-args-to-session-constructor branch from 88bf9c8 to b772364 Compare April 29, 2021 19:03

thecodepixi requested a review from paulomarg April 29, 2021 19:07

paulomarg reviewed Apr 29, 2021

View reviewed changes

JaKXz reviewed Apr 29, 2021

View reviewed changes

Comment thread src/auth/session/session.ts Outdated

Remove default value for isOnline

65226ed

thecodepixi force-pushed the thecodepixi/move-required-args-to-session-constructor branch from 753a7e8 to 65226ed Compare April 29, 2021 19:28

Move changelog line to Unreleased

9c59dc2

arsnl mentioned this pull request Jul 29, 2021

Custom session storage generates multiple session IDs from single app install #224

Closed

hgezim mentioned this pull request Oct 13, 2021

Thecodepixi/move required args to session constructor #245

Closed

mllemango changed the base branch from main to v2 October 25, 2021 14:35

Merge branch 'v2' into thecodepixi/move-required-args-to-session-cons…

a8b313d

…tructor

mllemango merged commit 650ebe2 into v2 Oct 25, 2021

mllemango deleted the thecodepixi/move-required-args-to-session-constructor branch October 25, 2021 14:51

Conversation

thecodepixi commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

Type of change

Checklist

Uh oh!

paulomarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thecodepixi commented Apr 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sagar-ranglani commented Sep 17, 2021

Uh oh!

mariomx3 commented Oct 12, 2021

Uh oh!

hgezim commented Oct 13, 2021

Uh oh!

mariomx3 commented Oct 15, 2021

Uh oh!

paulomarg commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thecodepixi commented Apr 28, 2021 •

edited

Loading