Cleanup #351

kulshekhar · 2017-10-21T07:22:05Z

Following the discussion in #349

All I've done so far is

remove __TS_CONFIG__ related stuff
rename some variables/parameters for clarity
move some functions, that are only used in our tests, from utils.ts to a new file test-utils.ts

closes #394
closes #395
closes #396

GeeWee · 2017-10-31T20:10:55Z

src/utils.ts

-      );
-    }
-    result = converted.options;
+  if (configPath === path.join(getStartDir(), 'tsconfig.json')) {


Is this to ensure that if the file is just called tsconfig.json we overwrite the module, and if it's called something like tsconfig.test.json we expect people to know what they're doing? I like the idea, but I feel like it could lead to trouble.

The thought process was that because most people don't create a custom tsconfig for testing, it's safe to override the module to commonjs during testing. But if they do have a custom tsconfig, they're more likely to have thought through what they want and any module change they made should be used

GeeWee · 2017-10-31T20:17:51Z

I wasn't quite sure how to proceed with this - as I wanted to comment on some code that wasn't in a diff. I settled for adding TODO's - as they're easy to find.

I didn't want to change your branch, but I figured it was the easiest way to get some talking points down. Overall I think this looks really promising.

What are your thoughts?

kulshekhar · 2017-11-01T04:55:38Z

ts-jest/src/preprocessor.ts

Line 24 in 9cfd056

    
           /* TODO: Consider inlining tsJestConfig here, so it's obvious it's only used here. Actually seeing as tsJestConfig

Consider inlining tsJestConfig here, so it's obvious it's only used here.

I see your point. I normally don't like to place function calls as arguments but since the arguments here are on new lines, I don't see much of a problem in doing this.

Actually seeing as tsJestConfig
is contained withing jestConfig, perhaps we should just have the getPostProcessHook extract it itself?

I think passing in the config makes it easier to debug (and add tests) getPostProcessHook should we have to.

ts-jest/src/preprocessor.ts

Line 33 in 9cfd056

    
           // TODO: Comment what's going on here. I'm not quite sure why we're adding this if it's an html file

This was added in a PR to support angular 2+

ts-jest/src/preprocessor.ts

Line 41 in 9cfd056

    
           // TODO: Consider flipping this boolean to exit-early if we don't want to process the file.

yeah, something like if (!processFile) return src right after fetching compilerOptions? I agree that this would make the code cleaner.

EDIT: Actually, it might just be better to move the creation of the postHook to the point where it is used, AND flip the boolean

ts-jest/src/preprocessor.ts

Line 57 in 9cfd056

    
           // TODO: Consider creating a function for this part? e.g. prependSourcemapHook()

Agreed.

ts-jest/src/utils.ts

Line 96 in 9cfd056

// TODO: This can take something more specific than globals

If we try to use something more specific (e.g. globals['ts-jest']), we'll have to do a null check on globals from wherever we call this function. I think the current state is a cleaner alternative

ts-jest/src/utils.ts

Line 118 in 9cfd056

    
           // TODO: Perhaps rename collectCoverage to here, as it seems to be the official jest name now:

instrument doesn't seem as clear as collectCoverage in the context of this function.

- inline the call to get ts-jest configuration - flip the check on the file type - extract the injection of the sourcemap hook into a function

GeeWee · 2017-11-02T15:24:07Z

I think passing in the config makes it easier to debug (and add tests) getPostProcessHook should we have to.

Fair enough.

This was added in a PR to support angular 2+

Can we link to the issue perhaps?

yeah, something like if (!processFile) return src right after fetching compilerOptions? I agree that this would make the code cleaner.
EDIT: Actually, it might just be better to move the creation of the postHook to the point where it is used, AND flip the boolean

Yeah something like that sounds good.

Let me know when I should take another look or if you want me to do some of the heavy lifting

kulshekhar · 2017-11-02T15:54:11Z

Let me know when I should take another look or if you want me to do some of the heavy lifting

I've already incorporated the earlier feedback. If you could add the link to the angular issue/PR, this PR would be in sync with our understanding of how things should be.

GeeWee · 2017-11-03T10:52:10Z

I've done that!

kulshekhar · 2017-11-03T11:30:11Z

Looks like the build is failing on Linux for Node 4.

Can you revert the tslint bit from this PR? That's unrelated to the cleanup and can be a separate PR, right?

kulshekhar · 2017-11-07T05:20:33Z

Looks like the build is failing on Linux for Node 4.

Can you revert the tslint bit from this PR? That's unrelated to the cleanup and can be a separate PR, right?

@GeeWee

GeeWee · 2017-11-07T07:14:06Z

Strange - yeah I just stumbled upon it as an annoyance when I was fixing something else.

I've removed the rule.

GeeWee

lgtm - not sure we should merge this in before our next major release - is there a chance we'll break things?

kulshekhar · 2017-11-08T11:37:23Z

We shouldn't merge this before the next major release - this PR removes support for __TS_CONFIG__

GeeWee · 2017-11-09T11:42:30Z

Good point. Nice work on this.

kulshekhar · 2017-12-18T13:29:19Z

@GeeWee this update seems to have broken something in hoisting. I've Skipped that test for now.

Can you please take a look?

kulshekhar · 2017-12-18T23:32:19Z

@GeeWee don't worry about it. Turns out, it was a bug in jest

kulshekhar and others added 12 commits October 21, 2017 11:27

Remove __TS_CONFIG__ related code/tests

8d2d264

Check file type using regex

2bdf87a

rename config -> jestConfig in preprocessor

e6c0e0a

Cache files after the modification is complete

0e8b0cc

Rearrange imports

4640d6d

Rename params

3bfb4f8

Move test specific utils into their own file

20d252a

Use fs-extra to create cache file

19d8ae5

unexport locally used function

4573cc8

Rearrange statements in the getTSConfig function

cfdc9a8

Merge branch 'master' into cleanup

429bfc8

Merge branch 'master' into cleanup

5d7e2bc

GeeWee reviewed Oct 31, 2017

View reviewed changes

added a bunch of talking points

9cfd056

kulshekhar added 2 commits November 1, 2017 10:45

Incorporate GeeWee's comments

bfa89f8

- inline the call to get ts-jest configuration - flip the check on the file type - extract the injection of the sourcemap hook into a function

minor refactor

d52bac9

added a tslint rule and a comment

954b943

GeeWee added 2 commits November 7, 2017 08:12

removed tslint rule

c9fa987

removed comma

69463d9

kulshekhar and others added 3 commits November 8, 2017 09:32

Merge branch 'master' into cleanup

94908b0

Fix import order (tslint)

c8e6242

Fix babel related tests (include json as valid extension)

00d87b8

Merge master

cfebfd9

GeeWee previously approved these changes Nov 8, 2017

View reviewed changes

kulshekhar and others added 3 commits November 11, 2017 14:38

Merge branch 'master' into cleanup

6489567

Merge branch 'master' into cleanup

b5bf64e

merge master

36aff99

kulshekhar dismissed GeeWee’s stale review via 36aff99 November 17, 2017 04:41

kulshekhar and others added 10 commits December 1, 2017 17:20

Merge branch 'master' into cleanup

78501f8

Merge branch 'master' into cleanup

ac1d987

disable greenkeeper-lockfile

0f4b56b

Update jest/related deps to 22

80ceefe

Drop node v4 support

c945603

Add 'yarn test' to travis commands

ee71dce

Run tests from the command line

efc8dbc

revert last change

540ad1b

Fix test errors. Skip hoisting test

bce665f

Bump version

43008fa

kulshekhar requested review from bcruddy, GeeWee and morajabi December 18, 2017 14:02

kulshekhar added 2 commits December 19, 2017 04:54

update jest version

a13e232

Restore tests to their original state

59af832

kulshekhar changed the title ~~[WIP] Cleanup~~ Cleanup Dec 18, 2017

kulshekhar merged commit c610c13 into master Dec 18, 2017

kulshekhar deleted the cleanup branch January 30, 2018 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup #351

Cleanup #351

kulshekhar commented Oct 21, 2017 •

edited

Loading

GeeWee Oct 31, 2017

kulshekhar Nov 1, 2017

GeeWee commented Oct 31, 2017

kulshekhar commented Nov 1, 2017 •

edited

Loading

GeeWee commented Nov 2, 2017 •

edited

Loading

kulshekhar commented Nov 2, 2017

GeeWee commented Nov 3, 2017

kulshekhar commented Nov 3, 2017

kulshekhar commented Nov 7, 2017

GeeWee commented Nov 7, 2017

GeeWee left a comment

kulshekhar commented Nov 8, 2017

GeeWee commented Nov 9, 2017

kulshekhar commented Dec 18, 2017

kulshekhar commented Dec 18, 2017

Cleanup #351

Cleanup #351

Conversation

kulshekhar commented Oct 21, 2017 • edited Loading

GeeWee Oct 31, 2017

Choose a reason for hiding this comment

kulshekhar Nov 1, 2017

Choose a reason for hiding this comment

GeeWee commented Oct 31, 2017

kulshekhar commented Nov 1, 2017 • edited Loading

GeeWee commented Nov 2, 2017 • edited Loading

kulshekhar commented Nov 2, 2017

GeeWee commented Nov 3, 2017

kulshekhar commented Nov 3, 2017

kulshekhar commented Nov 7, 2017

GeeWee commented Nov 7, 2017

GeeWee left a comment

Choose a reason for hiding this comment

kulshekhar commented Nov 8, 2017

GeeWee commented Nov 9, 2017

kulshekhar commented Dec 18, 2017

kulshekhar commented Dec 18, 2017

kulshekhar commented Oct 21, 2017 •

edited

Loading

kulshekhar commented Nov 1, 2017 •

edited

Loading

GeeWee commented Nov 2, 2017 •

edited

Loading