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

Use tsconfig.json instead of import #30

Closed
MaximeBernard opened this issue Feb 21, 2023 · 23 comments
Closed

Use tsconfig.json instead of import #30

MaximeBernard opened this issue Feb 21, 2023 · 23 comments
Labels
help wanted Extra attention is needed

Comments

@MaximeBernard
Copy link

MaximeBernard commented Feb 21, 2023

Since it's global like lib.dom.d.ts, it makes more sense in the compiler options imho.

{
  "compilerOptions": {},
  "include": ["@total-typescript/ts-reset"]
}

It also avoids having to choose in what TS file to put your import "@total-typescript/ts-reset";

@mattpocock
Copy link
Collaborator

I agree, this is a suggestion worthy of the readme.

But - wouldn't this, by default, exclude all the other files in your repository?

Is there a better recommendation we can make?

@KajSzy
Copy link

KajSzy commented Feb 21, 2023

I think adding whole package (or specific files) as one of typeRoots should do the job too.
https://www.typescriptlang.org/tsconfig#typeRoots

Ofcourse this requires to pass node_modules/@types explicitly

@mattpocock
Copy link
Collaborator

mattpocock commented Feb 21, 2023

Yep, which takes it out of contention for me. In a monorepo this is a nightmare. Same is true of types.

@Krisztiaan
Copy link

You can also alias this (or any other type-related) module's name to start with @types/.
In this case, just install this module like so: yarn add -D @types/ts-reset@npm:@total-typescript/ts-reset or npm i -D @types/ts-reset@npm:@total-typescript/ts-reset.

@mattpocock
Copy link
Collaborator

As mentioned in #34, I'd prefer not to alias it because it makes the behaviour of the library more mysterious. It also means you can't incrementally adopt rules.

@Krisztiaan
Copy link

I do see your concern, and agree it may not be the best idea to promote the aliasing as the primary way of using this quite handy library.

The way I also picture the majority of users for this library, a "quick and simple" way would likely still be preferred.

Module aliases are also supported in npm & yarn & pnpm for quite a while now.

In #34 not suggesting to break anything in the library, just suggesting something that is already very much possible, but not too visible to the general users of Typescript.

Probably a better solution would be for typescript to have typeRootsExtension or something similar.

As I don't see anything to suggest that's happening, there are other alternatives, one I'll probably be using, as it is more explicit, but still fits to how I like to configure projects:

{
  // tsconfig.json, added below `compilerOptions`
  "typeAcquisition": {
    "enable": true,
    "include": ["@total-typescript/ts-reset"]
  },
}

@rgolea
Copy link

rgolea commented Feb 21, 2023

After a while of searching, I found out that you just need to add this like to compilerOptions:

{
    "compileOptions": {
            "typeRoots": ["./node_modules/@types", "./node_modules/@total-typescript"],
            // ...
     }
}

The only problem with this, is that @mattpocock would have to kind of publish each library separately and people would just have to install the packages that they might need. Should we just get a @ts-reset workspace and publish like that? (please don't get the bad idea to get this namespace)

@mattpocock
Copy link
Collaborator

@rgolea This would also be extremely painful in a yarn-based monorepo

@Krisztiaan
Copy link

You could add package.json files to the submodules, to let the other ways of inclusion (like typeAcquisition) be able to split the types.

@rgolea
Copy link

rgolea commented Feb 21, 2023

So there's literally no way?? @mattpocock

@mattpocock
Copy link
Collaborator

I'm nervous about recommending this:

"typeAcquisition": {
    "enable": true,
    "include": [
      "@total-typescript/ts-reset"
    ]
  }

Because it appears to add some quite aggressive behaviour - it seeks more @types/* files than currently exist in the project. This config option is designed for JS apps, not TS apps, which makes sense why it's so aggressive.

Using this option, it's also not possible to incrementally add rules.

@mattpocock mattpocock added the help wanted Extra attention is needed label Feb 22, 2023
@mattpocock
Copy link
Collaborator

mattpocock commented Feb 22, 2023

Added the Help Wanted tag as I can't seem to find a tsconfig option that covers all our cases.

Summary:

  • types and typeRoots, when specified, mean you need to specify all type roots/type sources.
  • typeAcquisition is too aggressive, and not designed for this purpose.
  • Creating a TS plugin means ts-reset wouldn't contribute to type checking.

@mattpocock
Copy link
Collaborator

This tweet from an esteemed ex-colleague is enough to make me convinced that this issue can be closed.

@robpalme
Copy link

TS-reset is effectively complementing and overriding the default built-in global environment. In tsconfig, that environment is most closely selected and defined via lib.

https://www.typescriptlang.org/tsconfig#lib

@Andarist
Copy link

Andarist commented Feb 22, 2023

ye, but there is no example there for providing a custom lib. OTOH, I vaguely remember that it was supposed to use a custom DOM lib since one of the more recent versions...

EDIT:// it's here: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#supporting-lib-from-node_modules . To make it work you'd have to reexport the original lib with some extra additions and I'm not sure how to really access the original lib file - since it usually comes from a global. Sure, it exists on disk - but I would consider this to be a private filename (maybe I shouldn't?)

@orta
Copy link

orta commented Feb 22, 2023

Re: custom lib dts - assuming this code only has additions to the global scope, you could pick a reasonably empty lib .d.ts and use the overrides to include the original .d.ts contents and then all your additions, then the recommendation is something like "@typescript/lib-es5": "npm:@ts-reset".

You probably want to avoid lib because including 1 thing means you dont get the default behavior, and have to include the right target + dom etc

Without thinking too hard about this problem because I'm low on time, I'd probably see if type roots with your own npm namespace and then one dependency which sets the globals should probably do it because everyone uses node_modules so including that in the instructions is ok

@sbyware
Copy link

sbyware commented Feb 22, 2023

I do agree that a specific import file should be specified, as right now it's pretty ambiguous on where you should/can import it (anywhere) and where it actually applies (everywhere). Importing into some arbitrary typescript file and having the import modify your entire project isn't clear, whereas we could possibly add a reset.config.ts? This way, we can import in a single location and define configuration all in one place, such as if we only want parse only etc. An example config file:

import "@total-typescript/ts-reset";
import type { Config } from "@total-typescript/ts-reset";

const config: Config = {
    jsonParse: true,
    fetch: false // can be an optional prop, so you explicitly state false but if not provided will be true
}

export default config;

Just a thought!

@mattpocock
Copy link
Collaborator

@sby051 Thanks for the suggestion, but I don't see a path for actually making it work and I don't think it would be preferable to just using an import.

@sbyware
Copy link

sbyware commented Feb 22, 2023

@sby051 Thanks for the suggestion, but I don't see a path for actually making it work and I don't think it would be preferable to just using an import.

Completely understood, although I do still think it should be more obvious on where you should import it (for example, I'm using it in a SvelteKit project right now and importing it in my root types.ts file as it seemed most appropriate, but you get what I mean). And its still a bit ambiguous especially to someone who may be unfamiliar with the package seeing such an import and subsequently wondering why it's applying to the entire project.

@mattpocock
Copy link
Collaborator

@sby051 I've changed the readme to say you should use a resets.d.ts file

@sbyware
Copy link

sbyware commented Feb 22, 2023

@sby051 I've changed the readme to say you should use a resets.d.ts file

Brilliant! A lot clearer what it's doing and where to put it in a codebase.

@tiagobnobrega
Copy link

Could this be used in a triple slash directive? Sometimes people already have global overrides definition file. Just adding the directive there could be cleaner.

@theKashey
Copy link

There are no real reason to use “types”, you can use “files” to emulate import of ts-reset, and I would say - files working way more predictable than other options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.