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

break circular references by splitting some files into two #747

Closed
wants to merge 1 commit into from

Conversation

leemhenson
Copy link
Contributor

Hi

I've been encountering an issue which occurs when using fp-ts on a large codebase, where one package using fp-ts imports from another package also using fp-ts. The sympton is that tsc crashes with an out of memory error. It is definitely linked to fp-ts, as compiling the project without fp-ts does not crash. I believe it is due to the circular references that exist in the codebase. Sadly the issue is not reproducible using a small repo (https://github.com/leemhenson/fp-ts-oom-repro), it only manifests when the codebase is large enough. My guess is that the size of the codebase trips tsc into some other mode.

To fix it, I have branched off 1.14.2 and:

  • added a tslint circular reference rule
  • fixed all the circular reference violations by splitting those files into two, e.g. Option.ts and Option_.ts. The underscore version contains the "meat" of the type without imports that cause the circular references, while the non-underscored version has those imports and the functions that use them, and also re-exports from the non-underscored file. This helps to preserve the same interface to anyone currently importing from, say, Option.ts.

Using a git reference in the package.json to a compiled version of my branch seems to resolve the issue.

I fully expect you will have preferences in how you would like to structure the code rather than the underscore approach I've used, this is just a proof of concept that shows it is possible to break the cycles.

@gcanti
Copy link
Owner

gcanti commented Feb 22, 2019

Hi @leemhenson

where one package using fp-ts imports from another package also using fp-ts

Did you check for multiple versions of fp-ts (which is known to cause tsc to hang during compilation)?

The sympton is that tsc crashes with an out of memory error
My guess is that the size of the codebase trips tsc into some other mode

I would recommend to also file an issue in the TypeScript repo.

Sadly the issue is not reproducible using a small repo (https://github.com/leemhenson/fp-ts-oom-repro)

Not sure I'm following, does the linked repo contain a repro?

@leemhenson
Copy link
Contributor Author

Did you check for multiple versions of fp-ts (which is known to cause tsc to hang during compilation)?

It is pinned to the same exact version (1.14.2) in both repos, and I checked with yarn why fp-ts to ensure a single version was being selected. It's possible this is the same issue as that multiple-versions problem. If we can break the circular refs, I would be interested to install a vNext and vNext+1 in the same project to see if that issue was resolved also.

I would recommend to also file an issue in the TypeScript repo.

If I could put together a repro I would. 😬

Not sure I'm following, does the linked repo contain a repro?

Eh, no. I was just showing how our project is roughly structured (e.g. the nesting). Sadly I can't make it fail with this small repo size.

@leemhenson
Copy link
Contributor Author

Any thoughts about what to do with this issue? There is lots of anecdotal evidence that circular dependencies cause a variety of unexpected silent or not-so-silent issues. Typescript's issue tracker has lots of hits, including this one added in just the last week: microsoft/TypeScript#29997

@gcanti
Copy link
Owner

gcanti commented Feb 26, 2019

@leemhenson I'm sorry but we can't do much without a repro

@gcanti gcanti force-pushed the master branch 2 times, most recently from 93de5eb to 6f0676d Compare February 26, 2019 17:35
@joshburgess
Copy link
Contributor

joshburgess commented Feb 26, 2019

The times when I've encountered these sorts of issues were due to having a library like newtype-ts pulling in dependencies from a separate fp-ts ecosystem library (like monocle-ts) while also having that library (monocle-ts, in this case) installed in my project. The way I fixed this was using the resolutions field of package.json.

However, this is a workaround. The more ideal solution would be for fp-ts ecosystem libraries to require these libraries as peer dependencies with looser version requirements instead of directly depending on them. This would (hopefully) allow users to avoid accidentally having more than one version of the same library installed at the same time and running into problems due to there being multiple versions of the same types.

@leemhenson
Copy link
Contributor Author

Having done some more digging, I am leaning towards this being the same issue as:

microsoft/TypeScript#30050

The nested node_modules is the common trigger, I believe.

I have modified my fp-ts-oom-repro project mentioned above to use a circular-ref-free 1.14.1 in the outer project and a circular-ref-free 1.14.2 in the inner project. Compiling the outer project still causes the oom.

Digging further, I was wondering if the multi-version crash we know about is maybe due to the declare module statements that reopen the HKT declarations conflicting across versions. I tried changing the HKT.ts file to HKTv14_1.ts in the circular-ref-free 1.141. I then opened just the inner project that uses 1.14.2 in VS Code and added:

import { URI2HKT2 } from "fp-ts/lib/HKTv14_1";

It does not believe that to be an error, despite the version of fp-ts installed locally not having an HKTv14_1 export. The intellisense offers me two entries for every export from fp-ts, and for HKT I can import HKT or HKTv14_1. It looks like, for some reason, TS is discovering the node_modules from the parent directory even though it's happily already parsed the node_modules in the local directory.

I can't imagine this is intended module resolution behaviour for TS, so I'll open an issue over there.

@leemhenson
Copy link
Contributor Author

I've opened this issue: microsoft/TypeScript#30124

I'm going to close this one with the expectation that something will come out of that or the other related issues.

@leemhenson leemhenson closed this Feb 27, 2019
@leemhenson
Copy link
Contributor Author

@gcanti I managed to put together a simple repro of oom crash with fp-ts:

https://github.com/leemhenson/project-refs-oom-repro

Raised as an issue on TypeScript here:

microsoft/TypeScript#30429

@gcanti
Copy link
Owner

gcanti commented Mar 22, 2019

Thanks @leemhenson

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