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

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

Merged
merged 12 commits into from Nov 16, 2022

Conversation

lmiller1990
Copy link
Member

@lmiller1990 lmiller1990 commented Nov 14, 2022

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.

highlightIndexes: number[]
}

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

function setSpecFilter (specFilter: string) {
if (specStore.specFilter !== specFilter) {
specStore.setSpecFilter(specFilter)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@cypress
Copy link

cypress bot commented Nov 14, 2022



Test summary

192 0 26 0Flakiness 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 Flakiness
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

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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.

@amehta265 amehta265 marked this pull request as ready for review Nov 14, 2022
@lmiller1990
Copy link
Member Author

lmiller1990 commented Nov 15, 2022

... 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
Copy link
Contributor

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
Copy link
Contributor

marktnoonan commented Nov 15, 2022

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 lmiller1990 merged commit 206fdd5 into develop Nov 16, 2022
82 of 83 checks passed
@lmiller1990 lmiller1990 deleted the lmiller/move-code-to-app branch Nov 16, 2022
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.

None yet

5 participants