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(dashboard-frontend): only use Lit #822

Merged
merged 10 commits into from
Aug 31, 2024
Merged

Conversation

xandervedder
Copy link
Collaborator

@xandervedder xandervedder commented Aug 2, 2024

Removes Angular.

ToDo list:

  • Add routing.
  • Make authentication work.
  • Use Typed Inject for dependency injection. This is not going to work since browsers require a constructor without parameters.
  • Migrate home page to Lit.
  • Migrate repositories page to Lit.
  • Migrate report page to Lit.
  • Remove all references to Angular (including old code).
  • Add UX fixes in repositories page.
  • Add tests (similar to how the report does it).
    • Vitest-browser + playwright.
  • Clean up.
  • [ ]

Closes #483

@xandervedder xandervedder added the area: frontend Everything that has to do with the frontend of the dashboard label Aug 2, 2024
@xandervedder xandervedder self-assigned this Aug 2, 2024
@xandervedder xandervedder changed the title feat(dashboard-frontend): use Lit for the Dashboard as well feat(dashboard-frontend): only use Lit Aug 2, 2024
@hugo-vrijswijk
Copy link
Member

hugo-vrijswijk commented Aug 7, 2024

Wow! This is a lot of work 👏. I'm wondering if it makes sense to introduce some sort of static rendering. For instance, the homepage is just static content (except for the user icon if you are logged in). But it still requires loading the entire SPA. If Angular is already being removed, maybe it makes sense to look at something like Astro to statically build as much as possible, and keep Lit for any interactive elements/pages? Astro is build on Vite, so the setup should be similar. It will also pre-render Lit components to speed up page load for users, where possible. It's a different setup, because Astro is not a SPA. Components are dynamic, rather than the entire page

I have experience with this, so if you want to have a call to discuss options or a demo, let me know. Or tell me if you vehemently disagree 😅

@xandervedder
Copy link
Collaborator Author

I like the idea, but I am not sure how much effort it would take to port it to Astro. I see that they have a Lit guide: https://docs.astro.build/en/guides/integrations-guide/lit/. It might be wise to finish up this up first, what do you think?

I'm also going to improve the repositories pages so we can finally release it 😅.

I'm available next friday - for the entire day 😃.

@xandervedder xandervedder marked this pull request as ready for review August 19, 2024 18:13
@xandervedder
Copy link
Collaborator Author

xandervedder commented Aug 19, 2024

Not really ready, but I wanted to see the result on acceptance first 🙂.

It appears I need to fix the conflicts first.

@xandervedder xandervedder force-pushed the feat/use-lit-in-frontend branch 2 times, most recently from 5922526 to f029cc3 Compare August 22, 2024 18:51
@xandervedder xandervedder force-pushed the feat/use-lit-in-frontend branch from f029cc3 to 56f297d Compare August 22, 2024 18:55
@xandervedder
Copy link
Collaborator Author

xandervedder commented Aug 22, 2024

@hugo-vrijswijk It's on acceptance now: https://stryker-dashboard-acceptance.azurewebsites.net

It might get overwritten soon though (and yes, I will fix the e2e tests and add the other tests when I figure out how.. :))

@xandervedder
Copy link
Collaborator Author

Noticed a bug when fetching with relative URLs:
afbeelding

@hugo-vrijswijk
Copy link
Member

hugo-vrijswijk commented Aug 23, 2024

Noticed a bug when fetching with relative URLs: afbeelding

URL's don't accept relative URL's. You can just use a string instead of URL constructor, or create one with the origin of the current page:

new URL('/api/reports/github/bla/bla/bla', window.location.origin)

Looking at where the error comes from, you could either change packages/common/src/Uri.ts to not use the URL object, or use window.location.origin in packages/website-frontend/src/contract/constants.ts

Copy link
Member

@hugo-vrijswijk hugo-vrijswijk left a comment

Choose a reason for hiding this comment

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

Really awesome work! I have some minor code comments. I think we should probably add this extension to the recommended extensions as it caught some bugs (and remove the angular one).

Ive gone through most of the changes. Just not the tests. I think those could use somve love :)

packages/stryker-elements/src/lib/atoms/copy-text.ts Outdated Show resolved Hide resolved
packages/website-frontend/src/app.ts Outdated Show resolved Hide resolved
packages/website-frontend/package.json Outdated Show resolved Hide resolved
packages/website-frontend/package.json Outdated Show resolved Hide resolved
@xandervedder
Copy link
Collaborator Author

xandervedder commented Aug 24, 2024

Things left to do:

  1. Remove defineElement stuff.
  2. Add tests (in the same way as the report).
  3. Fix the typing output from dts (not sure why it broke in this branch).
  4. Fix E2E tests.

After that, I'd say we'll be in a pretty good place 😄

@xandervedder
Copy link
Collaborator Author

Still a bit puzzled why expect(true).to.equal(false); is not failing...

@xandervedder
Copy link
Collaborator Author

@hugo-vrijswijk I'm having trouble controlling (or mocking) window.location, do you have any idea how to go about that?

I see that it might be related to how the tests are run in Playwright..

@hugo-vrijswijk
Copy link
Member

@hugo-vrijswijk I'm having trouble controlling (or mocking) window.location, do you have any idea how to go about that?

vi.stubGlobal doesn't work? It might be because the tests are actually running in a browser, where window.location is a "special" global (changing it navigates away from the page). But what is it still used for? I thought all API calls could be done with relative url's?

@xandervedder
Copy link
Collaborator Author

For example: to get the current report, the slug is extracted from the URL (report.page.ts). I suppose we could ignore it or make a wrapper around it so I can mock it.

fetch-ing is mockable, so that's not an issue 🙂

@xandervedder
Copy link
Collaborator Author

I'm pretty much done here, but the E2E tests are a bit flaky it seems...

hugo-vrijswijk
hugo-vrijswijk previously approved these changes Aug 30, 2024
Copy link
Member

@hugo-vrijswijk hugo-vrijswijk left a comment

Choose a reason for hiding this comment

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

LGTM! Other than some minor comments. Congratulations on this massive rework 🙏

@xandervedder
Copy link
Collaborator Author

One issue with real-time reporting, I will fix that and after that it will be merged 😄

@xandervedder xandervedder merged commit 54f1cae into master Aug 31, 2024
3 checks passed
@xandervedder xandervedder deleted the feat/use-lit-in-frontend branch August 31, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: frontend Everything that has to do with the frontend of the dashboard
Projects
Development

Successfully merging this pull request may close these issues.

Use Lit instead of Angular in the frontend
2 participants