Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Non-App-Bridge Support #929

Closed
xplodedthemes opened this issue Aug 4, 2021 · 12 comments
Closed

Non-App-Bridge Support #929

xplodedthemes opened this issue Aug 4, 2021 · 12 comments

Comments

@xplodedthemes
Copy link

xplodedthemes commented Aug 4, 2021

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 ?

@ncpope
Copy link
Contributor

ncpope commented Aug 4, 2021

Hey @xplodedthemes, that's funny you mentioned this because earlier today I started a discussion about this here: #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.

@gnikyt
Copy link
Owner

gnikyt commented Aug 4, 2021 via email

@ncpope
Copy link
Contributor

ncpope commented Aug 5, 2021

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.

@gnikyt
Copy link
Owner

gnikyt commented Aug 5, 2021

@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
Copy link
Owner

gnikyt commented Aug 6, 2021

@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 gnikyt changed the title How to disable App Bridge and use as a Non Embedded app ? Non-App-Bridge Support Aug 6, 2021
@xplodedthemes
Copy link
Author

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

@gnikyt
Copy link
Owner

gnikyt commented Aug 6, 2021

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

@xplodedthemes
Copy link
Author

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

@ncpope
Copy link
Contributor

ncpope commented Aug 9, 2021

@xplodedthemes congrats on your first PR

@xplodedthemes
Copy link
Author

@xplodedthemes congrats on your first PR

Thanks! hope to do it more often!

@nyrsimon
Copy link

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

@Kyon147
Copy link
Collaborator

Kyon147 commented Sep 11, 2022

Closing in favour of #1208

@Kyon147 Kyon147 closed this as completed Sep 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants