laravel-shopify icon indicating copy to clipboard operation
laravel-shopify copied to clipboard

Non-App-Bridge Support

Open xplodedthemes opened this issue 3 years ago • 11 comments

Will the package support non embedded apps again ?

I understand that we need to use JWT session token going forward, however this applies to Embedded apps only, right ? Is there a way to support both independently ? For non embedded apps, simply redirect to the external app url and use the saved access token to make API calls ? Am I missing something ?

xplodedthemes avatar Aug 04 '21 20:08 xplodedthemes

Hey @xplodedthemes, that's funny you mentioned this because earlier today I started a discussion about this here: https://github.com/osiset/laravel-shopify/discussions/928

I think the issues tab would get more views though so I would prefer to have this discussion here if possible, whatever you prefer @osiset

Personally, I just don't think I would ever want to create an embedded app. I know there are some benefits but the tradeoffs are just too high in my opinion.

As far as the issue goes, I think the best place to start would be for us to create a new middleware or to re-implement the 'auth.shopify' middleware in some way. I think a middleware approach would be ideal because then we wouldn't have to set the app as 'embedded' or 'non-embedded' in the settings somewhere. That way the developer could even have a hybrid approach of embedded and non-embedded if they wanted.

ncpope avatar Aug 04 '21 21:08 ncpope

If someone wants to contribute the changes, I'm fine with that, I personally don't have any time this summer or fall to add an additional feature.

You'd have to essentially turn off the app bridge stuff in the Blade layout file if non-embedded. Then once loaded/installed, detect if you're in an iframe, break out of it.

For middleware, you'd need to create a new one, or use verify.shopify as a base class, and verify the shop via the logged in user through cookies. Since it won't be in an iframe, Itp shouldn't be an issue so the cookie stuff should go fine.

Other than that it should work then.. off the top of my head.

On Wed., Aug. 4, 2021, 18:55 Nathan Pope, @.***> wrote:

Hey @xplodedthemes https://github.com/xplodedthemes, that's funny you mentioned this because earlier today I started a discussion about this here: #928 https://github.com/osiset/laravel-shopify/discussions/928

I think the issues tab would get more views though so I would prefer to have this discussion here if possible, whatever you prefer @osiset https://github.com/osiset

Personally, I just don't think I would ever want to create an embedded app. I know there are some benefits but the tradeoffs are just too high in my opinion.

As far as the issue goes, I think the best place to start would be for us to create a new middleware or to re-implement the 'auth.shopify' middleware in some way. I think a middleware approach would be ideal because then we wouldn't have to set the app as 'embedded' or 'non-embedded' in the settings somewhere. That way the developer could even have a hybrid approach of embedded and non-embedded if they wanted.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/osiset/laravel-shopify/issues/929#issuecomment-892985655, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASO4OSOW7TTOVNAF3IOMPLT3GV55ANCNFSM5BR7J5LQ .

gnikyt avatar Aug 04 '21 21:08 gnikyt

I would be fine with working on this sometime this quarter. If someone wants to address it sooner please feel free to, that would be great.

@osiset Thanks for the overview.

I wonder if we would even need to break out of the iframe since Shopify apps only load in an iframe when they are embedded. That could be a step that we could probably just leave out, but it might be needed as I can't say with 100% certainty without some physical testing.

I'm not sure if we even need a base view.

ncpope avatar Aug 05 '21 22:08 ncpope

@ncpope awesome. From memory, it will still attempt to load it inside the iframe. A simple JS check on the window parent to the current window object and do a redirect if different should suffice.

gnikyt avatar Aug 05 '21 23:08 gnikyt

@ncpope Looked back at my old note... unless something has changed in the last couple years, the app will load in the iframe first yea, so a check is needed to break out.

My recommendation (off the top of my morning brain):

  • Move common functions between verify.shopify and the new middleware to a base middleware class (maybe, verify.shopify.external? or verify.external?)
  • Have verify.shopify extend this base middleware
  • Have the new middleware extend this middleware
  • Add logic for verify the shop via Laravel session

gnikyt avatar Aug 06 '21 13:08 gnikyt

Hey guys @osiset @ncpope, sorry took long to reply!

I have been working on this today and I think I got it working.

Now, based on the appbridge_enabled option, the correct middleware / auth method will be used.

For non embedded apps, just make sure it's also disabled within Shopify App settings as well, this way when clicking on the app within Shopify, it will load the external app URL.

Here are the changes that I made:

https://github.com/osiset/laravel-shopify/compare/master...xplodedthemes:non-embedded?expand=1

Everything seems to be working fine from my end. Please test from your end if you have some time. Let me know if you want me to create a pull request.

Thanks

xplodedthemes avatar Aug 06 '21 20:08 xplodedthemes

@xplodedthemes this is great!! I'd be fine with this going for a PR.. was hard to look over on mobile, but the only thing itching my brain is the use of shopsession again, I think it'll be confusing down the line using shopsession for non embed, and the session context (currently injected into the user model per request)... Maybe we can migrate shopsession to do the same/function the same as session context is doing now.. just a thought.. but yeah open a draft PR and we can discuss the nitty in there.

gnikyt avatar Aug 06 '21 21:08 gnikyt

@osiset Alright, PR opened! This is actually my first ever PR 🤦🏻‍♂️

xplodedthemes avatar Aug 07 '21 03:08 xplodedthemes

@xplodedthemes congrats on your first PR

ncpope avatar Aug 09 '21 21:08 ncpope

@xplodedthemes congrats on your first PR

Thanks! hope to do it more often!

xplodedthemes avatar Aug 10 '21 13:08 xplodedthemes

Hi - sorry for the newbie question, but I love this framework and would like to use it on a NON-EMBEDDED app - I see there is some work here. Did this ever make it into a release?? Thanks

nyrsimon avatar Jul 20 '22 22:07 nyrsimon

Closing in favour of #1208

Kyon147 avatar Sep 11 '22 07:09 Kyon147