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

Support Not applying strict* checks to node_modules / imported libraries. #44205

Closed
5 tasks done
icywolfy opened this issue May 21, 2021 · 9 comments
Closed
5 tasks done
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@icywolfy
Copy link

icywolfy commented May 21, 2021

πŸ” Search Terms

node_modules
exclude
alwaysStrict

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Allow compiler options that enable strict checking to only apply to project files only, and not imported modules.

[Edit 3] It is ideal if this can be expanded to suppress linter checks on node_modules, as well a strict checks.

[Edit 2]
A implementation proposal:
Provide a new configuration option --noStrictCheckOnNodeModule. This flag will prevent errors emitting on files referenced via **/node_modules/**. This may be further restricted to if the real-path of fire either within **/node_modules/** or outside the Project Root. Project Root may defined as the location of package.json. No additional configuration is possible. All strict mode checks are suppressed for these files.

πŸ“ƒ Motivating Example

A project imports a npm package, that contains .ts files.
Project compiles, and types are coherent.
Attempting to enable noImplictReturns, finds errors in the node_modules/ in an included project, and fails to compile.

The typing in the third-party library is correct: public static getTrigger(): undefined | "load" => { if (check()) return "load"; }
Though, the method has an implicit return.

I see no reason why the project should fail to compile, especially in this case, where the difference between return undefined and /* implicit return undefined */ is effectively a style issue with no impact on typing or behaviour. Doubly so, in that the errors are found in a third-party library, in code that is otherwise unused.

The above is a trivial example, but highlights the issue.

πŸ’» Use Cases

The primary use case, is to include strict type checking within a project.
The project in question is a library, and not an application.
The project was originally written with all strict checks enabled.
Over time, as more libraries publish only raw ts files (1), our project had to disable strict checks one by one, in order to compile.
At this point in time, the project is unable to enable any strict type checking, or linter checks at all, lest tsc report errors within the node_modules directory.

The main shortcomings, is that tsc can no longer be used to enforce strict adherence to guidelines, due to changes in third-party libraries. This has immediate impact on code quality. In particular, being unable to enable strictNullChecks on our library due to third-party library code, has negatively impacted code quality over time.

Currently, no workarounds have been found.
The best effort currently, is to check out a PR; enable all strict typings; sift through pages of reported errors, and attempt to find those not in node_modules. And then approve or deny based on whether any "real errors" were found. This leaves much room for error, and not all developers or reviewers may be diligent in finding a "real error". Whereas, it would be ideal for these sorts of errors to be reported at develop time, or on the build server.


(1) Against guidelines, an increasing number of libraries only publishing raw ts files, without providing any .js files. In these libraries, package.json refer to .ts files directly.

{
   "module": "./src/main.ts",
   "types": "./src/main.ts",
}

(2) This appears to be a common request on stack overflow, and elsewhere:


Change History:

  1. Added stack overflow links of other searching for the same problem.
  2. Updated description with clarification of scope.
  3. Added desire to include "linter checks" as well as "strict checks". I was unaware that noImplicitReturn was in a different category than Strict Checks.
@MartinJohns
Copy link
Contributor

This is very likely the same as #31035.

@icywolfy
Copy link
Author

icywolfy commented May 21, 2021

This is much more narrow in scope than #31035. Although, solving #31035 would indeed solve this issue.

The main differences:

  • I propose no per directory tsconfig files, or ability to configure rules separately for different directories. Behaviour within the source tree is to remain as is.
  • There is no ability to change configuration arbitrarily. The proposal is limited to strict mode checks only.
  • There is no ability to configure "custom" strict mode checks, all strict mode checks are fully disabled when the referenced file is located within node_modules/.

The aim is that these limitations narrow the scope enough for the feature to be implementable.

A minimal proposed configuration would be a new compiler option:
--noStrictOnNodeModules. Which, while processing any file, if the referenced (not resolved if symlinked) file location, is in a node_modules directory, all strict mode reporting is not applied.

Any additional configuration beyond this is out of scope.

As I am unaware of the internals of typescript, I omitted the following possible improvements from consideration:
--noStrictOnImports. Imported libraries, referenced by absolute path, have strict mode reporting disabled. This was disqualified as a file may be referenced by both absolute path, or by relative. In particular for monorepos; or linked packages.

--disableStrictForPath=[globPattern]. This adds complexity if the glob pattern is in the project files to be compiled. It is useful for disabling strict mode for testing, via **/__tests__/**. However, should the glob pattern capture a file in the project to be compiled, the strict check is effectively meaningless, and encourages bad design. The complication of determining whether a glob affects a project file to compile is unknown. This idea was disqualified as encouraging bad-design, and duplicating the existing feature of //@ts-ignore.

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript labels May 27, 2021
@RyanCavanaugh
Copy link
Member

The problem I have with this, above and beyond the concerns/limitations described in #31035, is that this is ultimately (at least for now) a pure configuration error. You might be able to flag your way out of some of the problems here but there isn't a solution that would, for example, stop tsc from trying to emit .js files from these non-.d.ts input files. The only correct path forward is to not have a .ts file in your node_modules.

The most straightforward escape hatch for this misconfiguration is something more like an option to say "all files outside my rootDir are fine but please treat them like .d.ts files"), but even that's a bit of a wreck in this case because you're going to pay a bunch of performance penalty every time doing inference on these files.

If you have folders that are separate compilation units that aren't external code (i.e. node_modules), project references already exist to enable the scenario of disparate compilation settings on separate parts of your project.

I don't think we should do work to enable a scenario like this that's ultimately doomed to be unacceptably suboptimal.

@icywolfy
Copy link
Author

icywolfy commented Dec 8, 2021

@RyanCavanaugh
After 6 months of attempting to pressure External Company to do something about their provided packages, they have pushed back effectively saying : we publish working TypeScript packages; there is nothing to fix. Dozens of other companies consume our packages with no issue. This is your problem not ours.

Is there -any- work-around, good-or-hack, that you can think of to allow us to enable strict: true in our build system, and for developers, so that code-quality can be enforced, while ignoring errors in third-party code that we have no control over?

Now, none of our projects can enable strict: true. Doing so would break the build due to an increasing number of third-party libraries of which we have no influence over, and whose owners have no interest in updating to meet strict.

The published packages are 100% TypeScript; the package.json entry point is index.ts. Only raw .ts files are shipped for client developers to use. These sdk files, node_modules/**/*.ts, do need to be complied to .js for our final bundle.

Issues in the third-party packages seem mostly minor, such as:

  • 21:16 - error TS2532: Object is possibly 'undefined'
  • 302:9 error TS2322: Type '(value?: void | PromiseLike<void> | undefined) => void' is not assignable to type '(value?: unknown) => void'.
  • 230:11 - error TS2322: Type 'null' is not assignable to type 'Timeout'.

There are seemingly hundreds of errors emitted against third-party libraries when strict is enabled.
However, having to manually enable strict mode for validation, and search for entries in our source tree is error prone, and difficult to enforce.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Dec 9, 2021

@icywolfy I don't know what to tell you; this is only one of many ways that a dependency that doesn't want to configure itself properly could cause downstream damage (e.g. they could also be doing something bad at runtime; this is out of everyone's control). If these are public packages I can try talking to them?

@bartvanheukelom
Copy link

I've done a bunch of cleanup of my code so that I could enable strict mode, only to find out that 1 library in node_modules ships as .ts instead of .d.ts, and does not compile in strict mode. Had I known in advance that this would be a problem, I might've found another library, but by now, it's already integrated into the project and has "poisoned the well", so to speak. Makes me sad that now I can't enable strict mode for just my own code.

@ritschwumm
Copy link

having been in a similar situation, i seriously considered adding a postinstall script which patches the libraries inside node_modules...

@aspiers
Copy link

aspiers commented Jan 8, 2023

I don't think we should do work to enable a scenario like this that's ultimately doomed to be unacceptably suboptimal.

What are the criteria for "unacceptably" here? It feels to me like this is at high risk of allowing the quest for perfection to become the enemy of the good. It's a lot better to be able to disable strict* checks in third-party code which is proven to behave well, than to be forced to disable it everywhere.

See also #40426 (comment)

@dawsonc623
Copy link

I came here through a web search, and I think this is what I am running into. It seems I have stricter configuration options enabled than some dependencies of mine (both direct dependencies or transitive ones). TypeScript seems to want to apply my rules to code it finds in node_modules - code that is not in my control nor developed with my rules in mind. Thus, I am getting compiler errors - ~90 of them, albeit if I fiddle around with combinations of exclude and skipLibCheck I can get it down to ~80.

I would rather not loosen my configuration to accommodate for code that is not mine, nor do I have any singular influence to tell every project that happens to end up a dependency of mine to develop to my standard. I imagine these projects have their own configurations that their code adheres to, so suspending technical feasibility for a moment would it be a decent idea to only apply the nearest configuration to any given TypeScript file?

I bring it up because the other options seem to be trying to strong-arm library developers into doing X or Y, and regardless of right or wrong in my experience no one takes kindly to trying to be strong-armed. Meanwhile, people who are just trying to work on their own TypeScript projects end up blocked or reducing their own standards due to some dependency that got pulled in somewhere along the way - unless they can fully track down where it came from and search for alternative libraries whose entire dependency tree adheres to the "rules". That latter bit seems like an unfair request to make of people who just want to use TypeScript in their project.

For me, this is a show-stopper as many of these errors come from transitive dependencies whose "ground zero" is a primary technology choice of mine. I am unwilling to just give up my tech choice over this issue, so my options are to shelve the project until resolution or to remove TypeScript from the project. Neither seem all that appealing, but a product built without TypeScript does seem marginally better than no product at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants