chore: move non-shared code from frontend-shared to app #24674

lmiller1990 · 2022-11-14T14:33:17Z

While working on the Spec List code for #24467 with @amehta265, we found it confusing that a lot of functionality was split across frontend-shared and app. Turns out a lot of what was in frontend-shared isn't shared - it's exclusively consumed in app. For this reason, I decided to move the app only code into app. This makes it much easier to follow the code around.

There are no functionality changes here, I just deleted some unused code and moved some from around.

lmiller1990 · 2022-11-14T14:34:20Z

packages/app/src/specs/spec-utils.ts

-  highlightIndexes: number[]
-}
-
-export function buildSpecTree<T extends FoundSpec> (specs: FoundSpec[], root: SpecTreeNode<T> = { name: '', isLeaf: false, children: [], id: '', highlightIndexes: [] }): SpecTreeNode<T> {


spec-utils ended up a dumping ground for random things - many of these functions have one very specific purpose, which is to build the spec tree (not just utils) so I moved them to a more accurately named file.

lmiller1990 · 2022-11-14T14:35:14Z

packages/app/src/specs/tree/useCollapsibleTree.ts

@@ -0,0 +1,266 @@
+import type { ComputedRef, Ref } from 'vue'


Moved from frontend-shared, no changes. This is not a shared file, it is only used in App.

lmiller1990 · 2022-11-14T14:41:43Z

packages/app/src/store/specs-store.ts

@@ -12,16 +12,12 @@ export const useSpecStore = defineStore({
  state (): SpecState {
    return {
      activeSpec: undefined,
-      specFilter: undefined,


Unused? I think a hold-over from before we used GraphQL and preferences to save the spec filter.

Yeah we don't seem to be reading it anywhere 👍

lmiller1990 · 2022-11-14T14:42:47Z

packages/app/src/composables/useSpecFilter.ts

@@ -25,7 +25,6 @@ export function useSpecFilter (savedFilter?: string) {

  function setSpecFilter (specFilter: string) {
    if (specStore.specFilter !== specFilter) {
-      specStore.setSpecFilter(specFilter)


Unused, this was not doing anything but saving the specFilter in the store for no reason.

cypress · 2022-11-14T15:08:09Z

Test summary

192 • 0 • 26 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`0fc7e6d`
Started	Nov 15, 2022 6:58 AM
Ended	Nov 15, 2022 7:08 AM
Duration	09:55 💡
OS	Linux Debian -
Browser	Chrome 106

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/global-mode.cy.ts		2
1	Launchpad: Global Mode > when projects have been added > updates most-recently opened project list when returning from next step
2	Launchpad: Global Mode > error state > should not persist the error when going back to the main screen projects

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

marktnoonan · 2022-11-14T20:15:24Z

packages/app/src/specs/SpecsList.vue

 import RowDirectory from './RowDirectory.vue'
 import SpecItem from './SpecItem.vue'
-import { useVirtualList } from '@packages/frontend-shared/src/composables/useVirtualList'
+import { useVirtualList } from './tree/useVirtualList'


Since this has been backported to VueUse, do we want to try using the VueUse implementation: https://vueuse.org/core/usevirtuallist/#usevirtuallist

I wonder if we have diverged or if this will just work.

Nice, we should do this. Not right here, though -- I don't want to introduce any risk or change to code in this PR where possible - this is a quick reshuffling of things.

marktnoonan · 2022-11-14T20:33:06Z

Nice to clean some of this up! I have no problems with this move, though I had been thinking in an almost opposite way - that possibly more things should begin their life in frontend-shared since extracting things from one of the other packages was relatively painful and time consuming recently around auth/cloud stuff. Had to touch a lot tests and components just to use something we had already built and get it stable again everywhere.

This wouldn't happen if we saw the app and launchpad packages as sort of thin wrappers, and had frontend-shared as the core frontend source of components for both. Then we would always be able to reuse design elements in different contexts.

Not blocking though, just something I meant to bring up.

Is this ready for review? It's still in draft.

lmiller1990 · 2022-11-15T06:56:34Z

... that possibly more things should begin their life in frontend-shared since extracting things from one of the other packages was relatively painful ...

Why was it painful? I wasn't involved in this, but it should be as simple as moving and updating some imports. If it's not, our code is coupled to our build infrastructure - which means we should fix that. In this PR, I just did a bunch of dragging around VS Code and a few find and replaces.

This wouldn't happen if we saw the app and launchpad packages as sort of thin wrappers, and had frontend-shared as the core frontend source of components for both. Then we would always be able to reuse design elements in different contexts.

I do kind of agree with this. I think generic components are fine to include in frontend-shared. The main thing I'm moving here is useVirtualList, which may even go away once we update to the latest VueUse.

That said, the opposite end of argument would be "don't over abstract; refactor and make generic if you need it.

I don't feel too strongly about this either way, the main reason I made this PR is I think it'll make reviewing the upcoming Run All Specs changes a little easier, since we are probably touching that area quite a bit.

Is this ready for review? It's still in draft.

Moved to review!

rockindahizzy · 2022-11-15T15:06:10Z

Just adding my .02. I'm all for YAGNI, and keeping things as minimal as possible. However, I would suggest that we commit to starting the life of a component in frontend-shared. In addition to Mark's viewpoints, it also makes it much easier for a developer to discover an implementation if something already exists. We should be careful to not over generalize a component just because it lives here, though. Let the re-usablity be implemented as needed.

marktnoonan · 2022-11-15T18:43:07Z

Happy to approve, especially as a broken-out part of other work that makes reviews easier 👏

Why was it painful? I wasn't involved in this, but it should be as simple as moving and updating some imports. If it's not, our code is coupled to our build infrastructure

Moving the components was OK, the challenges were mostly around graphql data that the components we moved needed to access. Moving/renaming components meant updating a lot of GQL intercepts, for example, since the naming convent for fragments is tied to the component structure, and a few other things. We can chat more about it some time. Probably ties more into our test infra than the actual location of our files.

@rockindahizzy I'd be fine with "new components are created in frontend-shared by default" and not moving the old stuff right away. We can update the frontend-shared README to guide people to do this, and discuss whether the team agrees in that PR.

lmiller1990 added 8 commits November 14, 2022 14:02

move non shared code to packages/app

21736c5

move things around

ac3d554

fix bug

c1808a5

remove unused code

f94ba64

remove unused code

9171926

revert change

1fd79c8

unused

d9401c4

fix code

90e14b2

lmiller1990 commented Nov 14, 2022

View reviewed changes

lmiller1990 added 3 commits November 15, 2022 00:35

revert [skip ci]

edd606b

revert [skip ci]

19b00e6

update

22f6735

lmiller1990 commented Nov 14, 2022

View reviewed changes

marktnoonan reviewed Nov 14, 2022

View reviewed changes

amehta265 marked this pull request as ready for review November 14, 2022 21:01

Merge branch 'develop' into lmiller/move-code-to-app

0fc7e6d

ZachJW34 approved these changes Nov 15, 2022

View reviewed changes

marktnoonan approved these changes Nov 15, 2022

View reviewed changes

lmiller1990 merged commit 206fdd5 into develop Nov 16, 2022

lmiller1990 deleted the lmiller/move-code-to-app branch November 16, 2022 02:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move non-shared code from frontend-shared to app #24674

chore: move non-shared code from frontend-shared to app #24674

lmiller1990 commented Nov 14, 2022 •

edited

lmiller1990 Nov 14, 2022 •

edited

lmiller1990 Nov 14, 2022

lmiller1990 Nov 14, 2022

marktnoonan Nov 14, 2022

lmiller1990 Nov 14, 2022

cypress bot commented Nov 14, 2022 •

edited

marktnoonan Nov 14, 2022 •

edited

lmiller1990 Nov 15, 2022

marktnoonan commented Nov 14, 2022

lmiller1990 commented Nov 15, 2022 •

edited

rockindahizzy commented Nov 15, 2022

marktnoonan commented Nov 15, 2022 •

edited

		@@ -0,0 +1,266 @@
		import type { ComputedRef, Ref } from 'vue'

chore: move non-shared code from frontend-shared to app #24674

chore: move non-shared code from frontend-shared to app #24674

Conversation

lmiller1990 commented Nov 14, 2022 • edited

lmiller1990 Nov 14, 2022 • edited

Choose a reason for hiding this comment

lmiller1990 Nov 14, 2022

Choose a reason for hiding this comment

lmiller1990 Nov 14, 2022

Choose a reason for hiding this comment

marktnoonan Nov 14, 2022

Choose a reason for hiding this comment

lmiller1990 Nov 14, 2022

Choose a reason for hiding this comment

cypress bot commented Nov 14, 2022 • edited

Test summary

Run details

Flakiness

marktnoonan Nov 14, 2022 • edited

Choose a reason for hiding this comment

lmiller1990 Nov 15, 2022

Choose a reason for hiding this comment

marktnoonan commented Nov 14, 2022

lmiller1990 commented Nov 15, 2022 • edited

rockindahizzy commented Nov 15, 2022

marktnoonan commented Nov 15, 2022 • edited

lmiller1990 commented Nov 14, 2022 •

edited

lmiller1990 Nov 14, 2022 •

edited

cypress bot commented Nov 14, 2022 •

edited

marktnoonan Nov 14, 2022 •

edited

lmiller1990 commented Nov 15, 2022 •

edited

marktnoonan commented Nov 15, 2022 •

edited