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(mapper): implement project mapper and tsconfig convenience mapper #1805

Closed
wants to merge 7 commits into from
Closed

Conversation

martaver
Copy link

Summary

In another issue (#1698), we identified that a moduleNameMapper was required in order to get jest to play nice with project references when outDir or rootDir are used.

Doing this manually sucks, particularly in a monorepo where you have lots of jest.config.js files, so I wrote a moduleNameMapper to read project references and generate the corresponding map for jest.

Test plan

Tests are included in the PR.

Does this PR introduce a breaking change?

  • Yes
  • No

It only adds features, without changing any existing functionality.

Other information

I added a dependency on json5. My reasoning for this is that in order to read project references correctly, we need to read the name of the package from package.json, and also outDir and rootDir from tsconfig.json. If we're reading these files anyway, I figured we might as well use a loader that doesn't punish developers for having comments of trailing commas in their json files.

Also, I added a tsConfigToModuleNameMapper convenience helper, which does both paths and references mapping at the same time, to keep ts-jest users joyful.

I wonder if there's a way to roll this into the ts-jest preset?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 15, 2020

Hi, thank you for this proposal. In general, I like the idea. FYI, you can even create a custom jest module resolver, the concrete example is from nrwl nx.

Talking about the implementation, actually lots of things in your codes have been handled by TypeScript APIs, such as findConfigFile, readConfigFile and parseJsonConfigFileContent. So actually something like json5 is not required as well as the logic to find config file and read config file.

So if exposing helpers like tsConfigToModuleNameMapper and projectsToModuleNameMapper, it is better to follow the example from nrwl nx to utilize the most out of TypeScript APIs.

Regarding to the new helper tsConfigToModuleNameMapper, this one is kind of similar to current pathsToModuleNameMapper. I think it's better to consider merging these 2 into 1. It might be a breaking change (not really if it works like after change) but it won't be an issue.

@martaver
Copy link
Author

Okay, I think the e2e tests are fixed. If there are any more errors, I'll clean them up.

I also updated the code to use the typescript API to load tsconfigs, but json5 is still used for the package.json.

The only question that remains is: one big magic 'createModuleNameMapper' function, or keep them discrete and separate?

We could create one function that accepts various args, including

  • paths object (as per old api),
  • tsConfig object,
  • dirname to search for tsConfig from, and
  • path directly to tsconfig.

We could just call the relevant moduleNameMappers accordingly based on what configuration information is available.

What do you think?

Also, we should update the docs once we've settled on the API.

@coveralls
Copy link

coveralls commented Jul 15, 2020

Pull Request Test Coverage Report for Build 5413

  • 63 of 70 (90.0%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 92.216%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/config/getTsConfig.ts 7 8 87.5%
src/config/tsconfig-to-module-name-mapper.ts 12 14 85.71%
src/config/projects-to-module-name-mapper.ts 27 31 87.1%
Totals Coverage Status
Change from base Build 5360: -0.5%
Covered Lines: 1120
Relevant Lines: 1162

💛 - Coveralls

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 15, 2020

We could create one function that accepts various args, including

  • paths object (as per old api),
  • tsConfig object,
  • dirname to search for tsConfig from, and
  • path directly to tsconfig.

We could just call the relevant moduleNameMappers accordingly based on what configuration information is available.

What do you think?

Also, we should update the docs once we've settled on the API.

I like this one.

The new function can exist next to the old function and put warning message on the old function that it is deprecated and instruct users to use the new one. The deprecated one will be removed in v27.

Or can go for complete replacement of the existing function with this new function ?

The first choice seems to be safer.

@martaver
Copy link
Author

martaver commented Jul 15, 2020 via email

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 15, 2020

Yes let’s do that way for smoother migration

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 21, 2020

Hi, I think the way ts-jest deal config when working with project references need to be changed. But I get some difficulties to find the right approach. Do you have some ideas ?

Ideally, if ts-jest handles correctly tsconfig, #1698 can be resolved without the need of this PR

@martaver
Copy link
Author

martaver commented Jul 22, 2020

Well... the thing is that if I don't use outDir or rootDir in my tsconfig (i.e. if index is in the root of the project), then jest module resolution works fine. So it obviously knows what directory to look for the module in. If I add main to package.json that points to an outDir path, then jest is no longer able to find the entry point for the module.

That leads me to believe that ts-jest is not respecting outDir and rootDir correctly.

Module name mapper solves the problem, because I'm able to consider outDir and rootDir when generating the path to which the module should map. However, if ts-jest generated the artefacts correctly, you're right we probably wouldn't need it.

@martaver
Copy link
Author

I'm actually noticing that this mapper doesn't work in all cases.

For some reason, it only works when isolatedModules = true... something I didn't noticed was enabled in @mfellner's repo.

If I disable isolatedModuels I get tsc error: TS2307: Cannot find module '@mfellner/my-library' or its corresponding type declarations.

This matches the error that I get when I run tsc.

To correctly compile project references, however, tsc is supposed to be run in 'build mode', with tsc --build.

I'm not sure what the correct path forward here is.

I'm starting to think project references aren't the greatest idea.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 24, 2020

For me the concept is nice but it’s kind of confused. Build mode works differently from normal mode but idk how much different it can be.

I’m not certain resolving #1698 can solve your issue as well as @mfellner repo but it gives some hopes

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 24, 2020

Your work here can be actually reused partially for fixing #1698 so it is still quite helpful to me :)

@martaver
Copy link
Author

No worries, I'm glad it helped! I hope we have a more comprehensive solution to all this soon!

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 25, 2020

I checked ts-jest compilation process. paths from tsconfig wasn't actually used in the process of compiling ts to js. That's why moduleNameMapper is required.

A new feature can be that, when compiling ts to js, the compile output should contain the real path instead of path alias, that can solve your problem with moduleNameMapper. Still, need to read all project references to be able to find the correct mapping which might be tricky.

To alter the final output from compiler, for now I know that only AST transformers can do it. I'm not sure if TypeScript has an API for that without the need of AST transformers.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 25, 2020

In Angular there is a transformer seem to do things like that https://github.com/angular/angular/blob/d1ea1f4c7f3358b730b0d94e65b00bc28cae279c/packages/compiler-cli/src/ngtsc/transform/src/alias.ts

Assume it works for normal cases, the rest is about gathering the correct paths configuration for project references

This feature can be separated into 2 steps: first make it work without project references. The next phase is support project t references.

If you are interested to help, PR is welcome :)

@martaver
Copy link
Author

I don't think that moduleNameMapper is necessarily a bad solution for this... Let me think out loud for a little bit...

Obviously it would be handier to not have to use it... But the way I look at it, for a normal typescript build, e.g. in tsc --build will follow the project references for build, but to execute the built output, node still has to be able to find the packages and it doesn't know anything about project references. Project references are just a feature of compilation. This is where something like yarn workspaces comes in. They can ensure that whenever node looks up a project's package import, it will find it in node_modules, because yarn has symlinked them all there.

Now jest doesn't know anything about project references either, but ts-jest will compile ts files into js, so jest can run them. This is different to the tsc build and build output goes into a jest-specific directory. Like node, at execution time jest will look for the project package references in node_modules but won't find them (unless you've got yarn workspaces and actually have built output sitting wherever main in package.json points to).

Only we don't want to have to run tsc to build all project references just to be able to run tests. So I guess there are two ways to do that: Either, we get ts-jest to redirect project package references to ts-jest's build output in the jest cache, or we use a moduleNameMapper to redirect the module to the TS entry point of the project package reference, which jest will then again invoke ts-jest on to compile and use the outputs of that.

So actually, the moduleNameMapper would probably end up doing duplicate work, since ts-jest is actually compiling the project references in the first place.

Yeah, I think if ts-jest could use a transformer to map the project package references to its own build output, that would really be the cleanest solution!

@martaver
Copy link
Author

Ah great!

If there's already a transformer, then you can actually compose the project reference output path by looking up main from package.json and outDir and rootDir from tsconfig.json. I do this in my PR, hang on I'll link the file.

@martaver
Copy link
Author

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 25, 2020

Probably I can check how Angular handles project references as in Angular 10 solution styles is supported.

For now, the 1st step to have a transformer to do convert alias to real path is already a handy feature.

The whole project reference thing is like replicating the behavior of tsc —build which might take more time.

@martaver
Copy link
Author

Doesn't ts-jest already follow project references and build them?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 25, 2020

Partially using. ts-jest only reads tsconfig to gather compilerOptions for compiling process. However, the gathered config is one single object which doesn’t respect different configs per project when using project references. Compiling isn’t the same as building.

My potential idea is that, this compilation process needs to be aware of the file belongs to which project when project references is used. Right now one single LanguageService is used to compile because only one single compilerOptions object.

If there are multiple compilerOptions objects (the case of project references), it means that there are the same amounts of LanguageService instances needed to do compilation.

I have asked in TypeScript Discord but still no right solution for that, just still my idea. One thing for sure is compilation needs to be similar to tsc build

@martaver
Copy link
Author

I see, okay... it's far more complicated than I thought, then...

Possibly the moduleNameMapper solution is actually better then, because ts-jest would get invoked for each project reference?

The only problem with the moduleNameMapper solution at the moment is that typescript errors will cause it to fail... specifically if you run tsc then it will complain that project package references can't be found. However if you run tsc --build then it builds them all fine. For some reason setting isolatedModules to true affects this.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jul 25, 2020

The error can be solved with the transformer I believe. Because the output from compilation process (the js output which will be given to jest to execute) still contains the path alias instead of real physical path.

isolatedModule: true is a different story. It is called “transpiler”, kind of similar to Babel. It is purely converting ts to js but lack of other features like type checking, const enum etc.

By default, LanguageService is used. It is a service layer of TypeScript Program. Program is responsible for compiling, type checking, building and other features.

@martaver
Copy link
Author

Hey!

How's it going with getting ts-jest working with project references?

What do you want to do with this PR?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Aug 13, 2020

oh I haven't looked into it yet, but I think this PR can be closed. I just wonder if the changes display in this PR will still stay after you delete the branch ?

@martaver
Copy link
Author

This PR hasn't been merged, if that's what you're asking?

I also haven't implemented the updated API for the module name mapper that we discussed.

I was waiting to see what you came up with with the project references - obviously it would be better to just have them handled naturally by ts-jest.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Aug 13, 2020

Actually the discussion about solution is here #1648 (comment)

The general idea I have is

  • Modifying readTsConfig to handle projectReferences

  • Modifying LanguageService so that when encountering projectReferences, there might be multiple LanguageService instances to compile ts to js depending on which project the file locates

This PR has the part of retrieving tsconfig from different project references' tsconfig, which is necessary imo for the general idea above.

That is my idea, look a bit logical but I haven't tried it out yet.

@martaver
Copy link
Author

Cool. Yeah this module mapper is a bit of a hack for it tbh - it actually results in the referenced projects being compiled twice.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Aug 17, 2020

I think I might find the correct solution for project references.

If detecting from tsconfig that user is using project references, ts-jest should use build mode instead (-b). According to TypeScript doc, build mode works hand in hand with project references.

The idea is: instead of compiling file by file, ts-jest should use build mode to compile all at once. After that, when jest asks for js output for each ts, ts-jest can simply look into output build directory to get the output produced by build mode earlier.

Not sure how the performance will be, but at least that looks like the correct way to go.

There is one downside is this approach might not work well with parallel tests as first run, all workers will attempt to run build mode at the same time. Maybe should not run build mode for all files at once but run build mode per file like current situation.

@martaver
Copy link
Author

It's worth a try. Build mode has incremental recompilation too, but it's hard to say how that would integrate best with ts-jest though.

In fuse-box, there is custom code that walks project references when they're encountered and performs a build on it, sourcing the reference's tsconfig. It's completely possible to do it without using build mode as well.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Aug 18, 2020

I think if ts-jest can replicate the same behavior with tsc -b, that would be the best without custom codes needed.

@martaver
Copy link
Author

Cool :)

@ahnpnl
Copy link
Collaborator

ahnpnl commented Sep 4, 2020

I just merged a custom AST transformer for moduleNameMapper to master. The transformer might not work well with project references yet but there are rooms for improvements

If you are interested to try out, you can check out master and look at https://github.com/kulshekhar/ts-jest/tree/master/e2e/__external-repos__/path-mapping for the example.

Besides, I have looked a bit into replicating tsc -b, it is possible but doesn’t work well with parallel tests. In parallel mode, each worker will try to perform tsc -b on its own. It seems to be not possible to communicate between these workers (or something I don’t know yet).

The only way I can think of now about replicating tsc -b is by going through each referenced project to collect tsconfig, check which project a file belongs to, then compile with the options of that project.

@martaver
Copy link
Author

martaver commented Sep 6, 2020

Haha awesome!!! I'll have to check this out. I think this'll be far more convenient than the moduleNameMapper.

The only way I can think of now about replicating tsc -b is by going through each referenced project to collect tsconfig, check which project a file belongs to, then compile with the options of that project.

That's basically how we did it for Fusebox and it works fine. It's a simple approach that opens the door to easy caching. I don't know where I would begin trying to synchronise with some arbitrary tsc -b process. The only tricky part for project references is:

  • dereferencing symlinks to figure out whether references packages exist in a node_modules folder on disk or not (if not, then they are local and should be built with their local tsconfig)... this is for yarn link and yarn workspace compatibility... but I imagine you would already be doing this
  • calculating entry point based on package.json main, outDir and rootDir... but I think you mentioned that you can get the typescript API to do this for you. If not, then that logic is included in my PR, if it helps!

@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 30, 2020

I read an issue in ts-node and what I got from it is: project references is just a way of mapping for module resolution. It doesn't do anything about compiling with separate tsconfig options.

So what I understood is, it's sort of similar to jest moduleNameMapper and we need to replicate that logic for ts-jest.

@martaver
Copy link
Author

martaver commented Nov 3, 2020

It doesn't do anything about compiling with separate tsconfig options.

I'm not sure what you mean here... each project definitely gets compiled with its own tsconfig.json options in its own step.

Do you mean that as long as we compile each 'project' independently and correctly set their module paths, we should be good?

If that's the case, then my PR is probably fairly handy...!

@ahnpnl
Copy link
Collaborator

ahnpnl commented Nov 3, 2020

What I meant was about using compiler options. The compiler options will come from a single tsconfig.json only. This part won't have to implement because ts-jest is doing it right.

The actual implementation to support project references is implementing how to resolve the referenced import paths, a.k.a module resolution, based on the references config in tsconfig.

Your PR will be the core thing to implement that 😀 Generally speaking, project references and jest moduleNameMapper is a bit similar to each other.

@martaver
Copy link
Author

martaver commented Nov 3, 2020

I'm pretty sure tsc will respect each project reference's own tsconfig.json compiler options. Do you mean ts-jest does that too?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Nov 3, 2020

Actually ts-jest doesn't. I'm checking with ts-node team and I saw the discussion about project references there and they mentioned only about module resolution. They are working on supporting project references too so I follow closely to that.

However, things are still vague when coming to this concept. Needs some times more to check and understand fully.

This pull request was closed.
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

3 participants