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: bump TS to 3.7 #9145

Merged
merged 2 commits into from
Nov 10, 2019
Merged

chore: bump TS to 3.7 #9145

merged 2 commits into from
Nov 10, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Nov 6, 2019

Summary

3.7 is out 🎉 I haven't actually tried to run this, just opening up a PR 😀

https://devblogs.microsoft.com/typescript/announcing-typescript-3-7/

Test plan

Green CI?

@jeysal
Copy link
Contributor

jeysal commented Nov 6, 2019

@SimenB
Copy link
Member Author

SimenB commented Nov 7, 2019

@G-Rath
Copy link
Contributor

G-Rath commented Nov 10, 2019

I had a quick look into this for my own learning, but the more I did, the more I couldn't explain it, and got sucked into; I think @jeysal might be right: this looks like it might be a bug in TS.

This fixes it:

export const getEachHooksForTest = (test: Circus.TestEntry) => {
    const result: {
        beforeEach: Array<Circus.Hook>;
        afterEach: Array<Circus.Hook>;
    } = { afterEach: [], beforeEach: [] };
    let block: Circus.DescribeBlock | undefined | null;

    do {
        block = test.parent;
        const beforeEachForCurrentBlock = [];

        for (const hook of block.hooks) {
            switch (hook.type) {
                case 'beforeEach':
                    beforeEachForCurrentBlock.push(hook);
                    break;
                case 'afterEach':
                    result.afterEach.push(hook);
                    break;
            }
        }
        // 'beforeEach' hooks are executed from top to bottom, the opposite of the 
        // way we traversed it. 
        result.beforeEach = [...beforeEachForCurrentBlock, ...result.beforeEach];
    } while (block.parent);
    return result;
};

Here's a Playground Link for easy playing around :)

@G-Rath
Copy link
Contributor

G-Rath commented Nov 10, 2019

Confirmed it's a bug in TS.

It's been fixed and merged into master.

You can work around it for now by just doing const hooks = block.hooks;

The root reason is that TS creates a circular reference itself in attempting to check for exhaustiveness 😬

@SimenB
Copy link
Member Author

SimenB commented Nov 10, 2019

Thanks @G-Rath! I'll apply the workaround locally and push 👍

@SimenB
Copy link
Member Author

SimenB commented Nov 10, 2019

Wait, what's the fix? const hooks = block.hooks; does't change anything, and the first thing you pasted always does block = test.parent; which would end up in a loop, no?

@G-Rath
Copy link
Contributor

G-Rath commented Nov 10, 2019

Wait, what's the fix? const hooks = block.hooks; doesn't change anything,

Sorry I should have been clearer: You don't use hooks - it's literally the assignment of the variable before the loop causes TS to use a slightly different analysis path that does't create a circular reference.

Alternatively, doing

        if (!block.hooks) {
            break;
        }

before the look would work too, and at least is doing something, even if it's a conditional that'll never be true.

the first thing you pasted always does block = test.parent; which would end up in a loop, no?

It shouldn't, b/c the do { block = block.parent; .... } while(block.parent) should mean it bails when the parent isn't defined.

I might have missed something, but that should be the unrolled version of do { ... } while ((block = block.parent))

I forgot that the first assignment is test.parent - my bad!

@SimenB
Copy link
Member Author

SimenB commented Nov 10, 2019

Found a tweak that was relatively clean 🤞

@codecov-io
Copy link

Codecov Report

Merging #9145 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9145   +/-   ##
=======================================
  Coverage   64.83%   64.83%           
=======================================
  Files         278      278           
  Lines       11732    11732           
  Branches     2882     2882           
=======================================
  Hits         7607     7607           
  Misses       3508     3508           
  Partials      617      617
Impacted Files Coverage Δ
packages/jest-circus/src/utils.ts 15.38% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7a7b42...2630dac. Read the comment docs.

@SimenB SimenB changed the title chore: bump TS chore: bump TS to 3.7 Nov 10, 2019
@SimenB SimenB merged commit 129c200 into jestjs:master Nov 10, 2019
@SimenB SimenB deleted the ts-37 branch November 10, 2019 08:38
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants