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(apps): frontend apps #9831

Merged
merged 109 commits into from
May 20, 2022
Merged

feat(apps): frontend apps #9831

merged 109 commits into from
May 20, 2022

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented May 18, 2022

Changes

2022-05-20 00 48 50

How did you test this code?

  • WIP

@mariusandra mariusandra requested review from Twixes and removed request for Twixes May 18, 2022 08:27
@mariusandra
Copy link
Collaborator Author

Definitely ready for a review now.

to={urls.plugins()}
/>
)}
<SideBarApps />
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocker: just leaving this here based on our sync discussion - when (under our current setup) do I see apps but can't use the apps browser/config page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 , bug fixed

The apps themselves should remain accessible for all users, even if they can't see the "browse apps" page for whatever reason (all I know is the API can say you have no access to the page 🤷). In this restricted case, an admin sets up the apps, and you as a user can then use them.

We'll need to go granular than that eventually, but not in this feature flagged v0.1.

key={id}
icon={<IconExtension />}
title={title || `App #${id}`}
identifier={currentLocation.pathname === urls.frontendApp(id) ? Scene.FrontendAppScene : 'nope'}
Copy link
Contributor

Choose a reason for hiding this comment

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

worth a comment, by your own admission @mariusandra this is "dodgy"


obj: Dict[str, Any] = {}
if not plugin_source:
obj = {"no_frontend": True}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I personally would pass a "status" e.g. status: no_frontend

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

obj = {"no_frontend": True}
if ('no_frontend' in app || 'transpiling' in app) { ... }

vs

obj = {"status": "no_frontend"}
if (obj.status === 'no_frontend' || obj.status === 'transpiling') { ... }

Dunno... 🤷 😆

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

A few minor comments but overall this is looking good!

As for the open questions:

  • I would say for now @posthog/apps-common is good enough. We should make it clear that this is a beta feature and the APIs may change, but this part definitely can be rearchitected easily.
  • As for local development, we do already have a local filesystem installation option - perhaps we could just make this work well with frontend apps? It'd actually make it easy to use things like AdHocInsight, as opposed to a separate create-react-app-like env.
  • I concur we need to expose some type definitions:
    types
  • Showing the header automatically is definitely safer. If it turns out it needs to be disabled in some situation, we can make that configurable in plugin.json.

? {
identifier: 'app-menu',
onClick: () => setOpenAppMenu(openAppMenu === id ? null : id),
popup: {
Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to put any other options in this menu? If, not I guess it could be just an "Edit" icon

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think yes. I also think having a lot of edit icons below one another makes for a very strong visual "look at me" signal that we want to avoid. A subtle menu is better.

Some things we can add in the menu:

  • Unpin from sidebar (and move to a "browse apps" dropdown menu, similar to "pinned" dashboards now)
  • Uninstall
  • Configure

@mariusandra
Copy link
Collaborator Author

mariusandra commented May 20, 2022

Most things addressed. Ready for another look!

I also added a "kill-switch". Now if the flag is turned off, no apps are loaded at all. Previously they just weren't shown in the sidebar, but we did still import and load the code. Now no longer. You'll need to reload after enabling the flag, but it's worth the safety guarantees.

  • As for local development, we do already have a local filesystem installation option - perhaps we could just make this work well with frontend apps? It'd actually make it easy to use things like AdHocInsight, as opposed to a separate create-react-app-like env.

Worth thinking about, and it could definitely help us... but I think "apps" should serve as an entrypoint to contributing to posthog without actually running the whole stack on your local dev box. Hence we still need some CRA-like setup.

  • I concur we need to expose some type definitions:

Absolutely. Beta stuff, this is.

  • Showing the header automatically is definitely safer. If it turns out it needs to be disabled in some situation, we can make that configurable in plugin.json.

Yup.

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Green check mark goes here

@mariusandra
Copy link
Collaborator Author

image

@mariusandra mariusandra merged commit 6bb8a6a into master May 20, 2022
@mariusandra mariusandra deleted the plugin-frontend-complete branch May 20, 2022 16:14
fuziontech added a commit that referenced this pull request May 20, 2022
* master:
  chore: start stack once in cloud tests (#9879)
  feat(apps): frontend apps (#9831)
  chore: Fix snapshots on master (#9885)
  chore(apps): rename plugins to apps (#9755)
  refactor: Remove constance library dependency, use json-encoded model (#9852)
  chore(clickhouse): avoid creating kafka_events, events_mv (#9863)
  fix(insights): Fix timezone date issues (#9678)
  refactor(plugin-server): refactor the event pipeline (#9829)
  feat(object storage): add unused object storage (#9846)
  fix: make kafka health check timeout test reliable (#9857)
  fix: query elements from start of day (#9827)
alexkim205 pushed a commit that referenced this pull request May 23, 2022
* create plugin source model

* edit source via plugin_source model

* deprecate "source"

* test plugin source updates

* add support for index.ts and index.js

* refactor plugin loading, support plugin sources from db

* fix source code in tests

* remove transpilation code

* reload plugin after saving

* store defaults in the db instead of persisting in form

* remove fields that don't exist

* feat(apps): transpile frontend.tsx

* update timestamp even if no local fields changed

* yarn.lock

* feat(apps): load apps with frontend.tsx in the frontend

* fix import

* reload apps, show if transpiling

* remove unused fields

* commit suggestion

* rename to PluginSourceFile

* rename to PluginSourceFile

* remove dead code

* cleaner types

* PluginSourceFile

* fix nag

* fix types

* one more merge conflict

* transpile when adding frontend

* fix test

* edit app from sidebar

* sneaky edit button

* fix code feedback

* add comments

* make it safer to call

* convert to upsert

* convert to upsert

* comment on the null

* revert plugin server changes

* get source from new model behind the scenes

* simplify

* revert accidental changes

* revert accidental changes

* remove defaults

* sync the old source field

* use the source model in the plugin server

* cache the source from pluginsourcefile

* test that getPluginSource gets data from the database

* safer null check that doesn't override `{}` in the db with `null`

* clarify origins

* use enum

* fix error from migration 0233 ([] is falsy in python!)

* fix merge

* error as null

* use the explicit "source__index_ts" field

* cache transpiled code

* remove unused type

* cleaner query

* remove extra line

* fix test

* improved error handling and run code through prettier

* new packages

* loading state in source editor

* kinder error

* sync plugin state if pluginsLogic, retry loading

* update snapshot after kea-forms update

* remove "updateAppConfig"

* add a query

* add examples to editor

* refactor sidebar

* add more lemons

* simplify app config

* rename action and add todo

* some helpful comments

* create appConfig if enabling a plugin and none was yet provided in django's app context

* re-enable app only if enabled

* fix last bug

* add back onInit and move types around

* show apps section header only if any link in the list

* remove dead code

* fix few errors, only allow editing of source apps

* stringify json

* completely bail out of apps if the flag is off

* Update snapshots

* fix broken link

* Update snapshots

* use new icon for "apps" now that it's not "plugins" any longer

* Update snapshots

* Update snapshots

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants