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(utils): adds waitFor util #39

Merged
merged 11 commits into from
Apr 26, 2022
Merged

feat(utils): adds waitFor util #39

merged 11 commits into from
Apr 26, 2022

Conversation

bezoerb
Copy link
Member

@bezoerb bezoerb commented Apr 15, 2022

No description provided.

@bezoerb bezoerb marked this pull request as draft April 16, 2022 07:18
@bezoerb bezoerb marked this pull request as ready for review April 16, 2022 21:53
@bezoerb
Copy link
Member Author

bezoerb commented Apr 21, 2022

Danke für's feedback

@bezoerb bezoerb requested a review from tharders April 23, 2022 21:20
Copy link
Contributor

@tharders tharders left a comment

Choose a reason for hiding this comment

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

Sieht schon ganz gut aus.
Schau noch mal auf die Kommentare, ob man da noch was machen muss.

packages/contentful-ssg/src/lib/utils.ts Show resolved Hide resolved
}
});

cyclicErrorObservable
Copy link
Contributor

Choose a reason for hiding this comment

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

Die cyclic error detection über ein observable laufen zu lassen kommt wir etwas zu komplex vor.
Werden damit async Probleme gelöst?
Ansonsten könnte man ja auch einfach direkt beim waitFor Aufruf als erstes buildDependencies aufrufen und prüfen ob ein Zyklus entstanden ist. Es muss ja nicht zwingend jeder waitFor der in dem Zyklus hängt eine Exception werfen. Es reicht ja der Aufruf, der den Zyklus schließ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.

ich hab mal ein bisschen ohne observable rumgespielt. Dann muss man ab der ersten cyclic dependency alles rejecten weil man ja nie weiß wer gerade für das schließen verantwortlich ist. Die transforms dauern ja im Zweifel alle unterschiedlich lang. Mit den Observables kann man das schön zuordnen :)

Copy link
Member Author

Choose a reason for hiding this comment

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

würde es daher drin lassen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

@bezoerb bezoerb requested a review from tharders April 25, 2022 23:07
tharders
tharders previously approved these changes Apr 26, 2022
Copy link
Contributor

@tharders tharders left a comment

Choose a reason for hiding this comment

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

Ein kleines wording in der readme ist mir noch aufgefallen, sonst können wir das so lassen.

###### waitFor

Wait for specific entry to be transformed.
Be aware that this can lead to dead ends when you're awaiting something which
Copy link
Contributor

Choose a reason for hiding this comment

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

Du meinst bestimmt "deadlock" und nicht "dead end" (Sackgasse)

@@ -115,6 +128,12 @@ export const run = async (config: Config): Promise<void> => {
await write({ ...transformContext, content }, ctx, config);
ctx.stats.addSuccess(transformContext);
} catch (error: unknown) {
if (error instanceof Error) {
subject.next({ ...transformContext, error });
} else if (typeof error === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ich hab überlegt, ob man den Fall abfangen müsste, dass der error kein Error und kein string ist.
Wäre ein bisschen robuster gebaut, aber wird vermutlich nie auftreten.

Copy link
Member Author

Choose a reason for hiding this comment

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

hatte ich auch erst drin, habs aber direkt wieder zurückgebaut.

@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@tharders tharders left a comment

Choose a reason for hiding this comment

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

Sieht jetzt gut aus

@bezoerb bezoerb merged commit 0840ecd into main Apr 26, 2022
@bezoerb bezoerb deleted the observable branch April 26, 2022 13:57
bezoerb added a commit that referenced this pull request May 10, 2022
* main: (23 commits)
  chore(release): updated release notes and package versions [ci skip]
  fix(observable): uses ReplaySubject for transforms
  chore(release): updated release notes and package versions [ci skip]
  Fix: avoid skipping link entries in submenus (#42)
  chore(release): updated release notes and package versions [ci skip]
  Fix: avoid skipping link entries in menus builder (#41)
  ci(tests): disable shallow clone in github actions workflow
  ci(tests): tweaks coverage exclusions in sonar.properties
  ci(tests): tweaks sonar.properties
  ci(tests): tweaks sonar.properties
  ci(tests): tweaks sonar.properties
  ci(tests): tweaks sonar properties
  ci(tests): adds sonarcloud to test.yml (#40)
  fix(tests): adds sonar reporter
  ci(tests): adds sonarqube to test.yml
  chore(release): updated release notes and package versions [ci skip]
  feat(utils): adds waitFor util (#39)
  chore(release): updated release notes and package versions [ci skip]
  Feature: enable custom hugo menu (#38)
  chore(release): updated release notes and package versions [ci skip]
  ...
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

2 participants