Port van @politie/sherlock tutorial #55
Conversation
Additionally fixed the broken imports of sherlock and sherlock-utils
Use npm run tutorial to run all the test cases in the tutorial directory.
Note that it's currently skipping the test cases. They have been validated to run (and fail) before committing though.
Again skipping by default.
There's still one part at line 121 where I'm not sure what's happening
As always, set to skip by default.
Some weird config/import spaghetti was going on. Now everything works with the '@' import notation. Co-authored-by: Paco van der Linden <[email protected]>
Currently skipped as always. Also removed some unused pieces of code: - Imports - Outer test suite that didn't do anything useful
Still have to iron out some details probably
Probably have to tweak it a bit more though
This way it doesn't count as a failed test case when running npm run tutorial.
Also updated a comment in tutorial 1 slightly.
tutorial/6 - errors.test.ts
Outdated
There was a problem hiding this comment.
Was een lege file, maar Jest geeft dan errors dat er geen test suites/cases in staan. Vandaar deze placeholder.
| * Now, use `pairwise()` directly in `.react()`. Implement the same | ||
| * derivation as before: subtract the previous value from the current. | ||
| * | ||
| * Instead of returning the computed value, assign it |
There was a problem hiding this comment.
Ik heb het een hele tijd geprobeerd, maar ik kreeg het niet voor elkaar om spioneren op het resultaat van .pairwise(). Als ik .react() vervang met een mock krijg ik namelijk alleen maar te zien dat deze aangeroepen is met een wrapped functie.
There was a problem hiding this comment.
Hetzelfde geldt overigens voor .scan() in de volgende test case.
| * | ||
| * This is best explained in practice. | ||
| */ | ||
| it('struct', () => { |
There was a problem hiding this comment.
Deze test heeft mij niet per se inzicht gegeven in waarom ik .struct() zou moeten gebruiken. Het voorbeeld laat namelijk alleen maar zien in welk opzicht .struct() op dezelfde manier reageert op een atom als op een normale member.
| */ | ||
|
|
||
| await new Promise(r => setTimeout(r, 1)); | ||
|
|
There was a problem hiding this comment.
Hier werd in de oorspronkelijke versie de mock gecleared na de delay. Deze variant zonder de clear lijkt me echter iets interessanter omdat je dan het resultaat van die delay ook kan waarnemen.
There was a problem hiding this comment.
Het bracht mij wel even van mijn a propos... Moet even kijken wat het duidelijkst is.
| describe('`derivableCache`', () => { | ||
| type Stocks = 'GOOGL' | 'MSFT' | 'APPL'; | ||
|
|
||
| let stockPrice$: jest.Mock<DerivableAtom<number>, [Stocks], any>; |
There was a problem hiding this comment.
Dit was eerst met een sinon stub functie geimplementeerd en dit was wel even puzzelen om goed om te schrijven. Volgens mij doet ie nu wat ie hoort te doen, kijkende naar de comments en de tests, maar even dubbel checken hier zou fijn zijn.
|
|
||
| beforeEach(() => { | ||
| // By default the stock price retriever returns unresolved. | ||
| stockPrice$ = jest.fn(_ => atom.unresolved()); |
There was a problem hiding this comment.
Hier jest.fn().mockReturnValue(atom.unresolved()); gebruiken werkte niet zoals verwacht, omdat het steeds dezelfde unresolved atom returnde.
| myCounter$.react(__YOUR_TURN__); | ||
|
|
||
| expect(lastPairwiseResult).toEqual(1); | ||
|
|
There was a problem hiding this comment.
Dit is denk ik de netste implementatie die ik kan bedenken. Zo schrijf je je code direct in de react, en heb je geen externe variabele.
Deze manier van kijken wat een reactSpy returned is common, dus lijkt me net.
| myCounter$.react(__YOUR_TURN__); | ||
|
|
||
| expect(lastScanResult).toEqual(1); | ||
|
|
There was a problem hiding this comment.
I think subtracting both in pairwise and scan shows the differences way better!
| * So try and remove the `cast`, see what happens! | ||
| */ | ||
| const c = constant('value') as unknown as SettableDerivable<string>; | ||
|
|
There was a problem hiding this comment.
What did the "as unknown" add?
| }); | ||
|
|
||
| it.skip('pairwise - BONUS', () => { | ||
| const myCounter$ = atom(1); |
There was a problem hiding this comment.
Just for the record: don't suggest to replace all these "copy the same implementation here" by a variable. The whole point of this is to show that using a variable works but not using a variable fails!
| * What do you expect this `Derivable` to do on `.set()`, `.get()` etc? | ||
| */ | ||
|
|
||
| // Remove this after taking your turn below. |
There was a problem hiding this comment.
Heb dit niet op alle plekken toegevoegd. Ik verwacht niet dat mensen op Run klikken voordat ze dit ingevuld hebbben. Bovendien, we gebruiken dit sowieso niet op alle plekken, dus misschien toch verwijderen?
Nickname5862
force-pushed
the
tutorial-base
branch
from
571e046 to
ec2aac7
Compare
Deze port doet voornamelijk twee dingen:
Het meeste was vrij straight forward. De wat grotere/complexere aanpassingen markeer ik met comments in Github.
Verder heb ik nog een aparte branch met mijn oplossingen voor de tutorials. Ik denk dat het handig is als die ook ter reference in de repo staan, misschien als een getagde commit? Voor nu zet ik het in ieder geval ook in een PR zodat het gereviewed kan worden (#56).