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

Ryddet opp i sirkulære dependencies #2366

Merged
merged 30 commits into from Oct 13, 2023
Merged

Ryddet opp i sirkulære dependencies #2366

merged 30 commits into from Oct 13, 2023

Conversation

KenAJoh
Copy link
Collaborator

@KenAJoh KenAJoh commented Oct 11, 2023

Description

I dag har systemet en del sirkulære dependencies som kan oppdages ved å kjøre

// Kjøres i @navikt/core/react
npx madge --circular --extensions ts ./src/ 

i tillegg til enda flere som ikke oppdages av "madge" siden de blir importert fra root index.ts.

PR-en rydder opp i alle disse.

Koden er da "klarere" til en potensiell oppdatering av exports (barrel exports) eller støtte av use client. For nå vil det ikke ha noen store påvirkninger for bruker. Ved lokalt bygg sparer jeg bare i snitt ~2-6 sekunder vs dagens løsning.

Endringer i konvensjoner

  • Contexter som brukes på tvers av parent/sub-komponent er trukket ut i egen fil context.ts
  • Komponent props som brukes på tvers av parent/sub-component er trukket ut i egen fil types.ts

TODO:

  • Fikse sikulær dependency i combobox før merge

@changeset-bot
Copy link

changeset-bot bot commented Oct 11, 2023

🦋 Changeset detected

Latest commit: 4690e97

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@navikt/ds-react Patch
@navikt/ds-css Patch
@navikt/ds-tokens Patch
@navikt/ds-tailwind Patch
@navikt/aksel-icons Patch
@navikt/aksel Patch
@navikt/aksel-stylelint Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2023

Storybook demo

920f1badd | 59 komponenter | 385 stories

@HalvorHaugan
Copy link
Contributor

Det finnes en eslint-regel for sirkulære deps hvis det er interessant: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Oct 12, 2023

Det finnes en eslint-regel for sirkulære deps hvis det er interessant: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md

Det har vært helt konge!

@HalvorHaugan
Copy link
Contributor

Vet du om det også er problematisk med sirkulær import når den ene importen bare er en type?

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Oct 12, 2023

Vet du om det også er problematisk med sirkulær import når den ene importen bare er en type?

Tror ikke det skal være et problem, bortsett fra at bygg og typecheck ta litt lengre tid når de er sirkulær. Men er ikke snakk om mange ms

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Oct 12, 2023

Det finnes en eslint-regel for sirkulære deps hvis det er interessant: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md

Testet den nå, fant en del flere feil da 🚀 Men yarn lint tar 3x så lang tid lokalt nå + alt er en del tregere når man endrer på komponenter. Så la den inn og kommenterte den ut, så kan vi bruke den til å gjøre litt sjekker i ny og ned 🤔

@HalvorHaugan
Copy link
Contributor

Det vi eventuelt kan gjøre, er å kun ta med den regelen når eslint kjører i workflow. Man kan modifisere reglene med cli option --rule, feks. yarn eslint --rule "import/no-cycle:error" .. Videre kan vi vurdere å kjøre test og lint parallelt.

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Oct 12, 2023

Det vi eventuelt kan gjøre, er å kun ta med den regelen når eslint kjører i workflow. Man kan modifisere reglene med cli option --rule, feks. yarn eslint --rule "import/no-cycle:error" .. Videre kan vi vurdere å kjøre test og lint parallelt.

Hvis denne fungerer yarn eslint --rule "import/no-cycle:error" virker det som en god løsning for workflows 👍 Kan lage en egen PR på det

@KenAJoh KenAJoh merged commit e49b7ec into main Oct 13, 2023
3 checks passed
@KenAJoh KenAJoh deleted the next-appdir branch October 13, 2023 08:06
@github-actions github-actions bot mentioned this pull request Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants