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

feat: ✨ add examples directory with .gitmodules #285

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

peelar
Copy link
Member

@peelar peelar commented Sep 8, 2023

@peelar peelar self-assigned this Sep 8, 2023
@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2023

⚠️ No Changeset found

Latest commit: 8caaeff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

Package Line Rate Branch Rate Complexity Health
src 83% 95% 0
src.APL 80% 85% 0
src.app-bridge 88% 86% 0
src.handlers.next 90% 84% 0
src.handlers.next.saleor-webhooks 93% 82% 0
src.middleware 60% 97% 0
src.settings-manager 96% 89% 0
src.test-utils 100% 90% 0
src.util 76% 100% 0
Summary 85% (3286 / 3878) 87% (398 / 455) 0

@peelar peelar marked this pull request as ready for review September 8, 2023 13:11
@peelar peelar requested a review from a team September 8, 2023 13:11
@lkostrowski
Copy link
Member

@peelar in general I like the idea, but I don't like that coupling

SDK depends on app-template, that contains SDK usage, on app-template branches, to mention this in SDK repo.

I think we should somehow move app-template code from app-template to app-sdk and keep them entirely here. Maybe submodules, maybe not, just I'd not decouple them.

Copy link
Member

@lkostrowski lkostrowski left a comment

Choose a reason for hiding this comment

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

Please figure out decoupling from app-template repo

@lkostrowski lkostrowski linked an issue Sep 23, 2023 that may be closed by this pull request
@lkostrowski
Copy link
Member

@peelar please hand over to @saleor/dex team

@peelar
Copy link
Member Author

peelar commented Oct 11, 2023

@saleor/dex

  1. We figured the App Examples are more like extensions of documentation of the App SDK, so we moved them here.
  2. In the next step, we wanted to turn Git submodules into next/examples style mini-apps right in the app-sdk repo. We found submodules difficult to work with, raising the entrance bar for somebody who wants to experiment with them.

We see that the issue with moving App Examples into SDK is that it is a bit awkward for CLI to consume bits of App SDK to spawn app templates. But maybe we should think of creating more domain-specific (like payments, taxes) app templates?

So this is our context. We are handing it over to you, but let's coordinate the fate of the app-examples repo together.

@typeofweb
Copy link
Contributor

Do we even need app-sdk examples if apps are open source already?

@lkostrowski
Copy link
Member

Do we even need app-sdk examples if apps are open source already?

IMO yes but if DX team decides not to have them, we can remove them.

I don't think apps are good examples because apps are complex and contain a lot of logic that hides what comes from SDK. Examples are just barebones app-template + app-sdk method applied on top. Very visible.

If we don't want to have examples to maintain, we should probably add more docs with examples written as snippets. Or - a code sandbox?

@typeofweb
Copy link
Contributor

Gotcha, we'll discuss this internally

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.

Migrate app examples to app sdk
3 participants