Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Internal improvement: Make consistent use of import type in most TS packages #4110

Merged
merged 20 commits into from
Jun 15, 2021

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Jun 15, 2021

In our Typescript packages, we've often been using import when we could be using import type. This PR is the start of an attempt at fixing that. Basically I went over most of the TS packages and replaced every import I could find that was only importing types with import type (I also did the same thing with exports where it seemed appropriate).

I did not touch codec or decoder because #3974 is still outstanding and I don't want even more merge conflicts.

I did not touch db or db-kit because they're large and confusing and I don't know them well, and I'm hoping someone else can handle those. Edit: According to @gnidan, these packages already do this properly, so I don't need to do anything there! Hooray!

I did not touch testing code, because I did not really see a need.

While doing this, I also noticed that:

  1. Outside of testing, @truffle/preserve-fs only uses @truffle/preserve for types, and
  2. Outside of testing, @truffle/preserve-to-ipfs only uses cids for types

So I reclassified both of those dependencies as devDependencies. The former seems a little weird, but that's what the code says! I don't know these packages really.

@rkalis
Copy link
Contributor

rkalis commented Jun 15, 2021

Wrt reclassifying e.g. @truffle/preserve as a devDependency in @truffle/preserve-fs:

@truffle/preserve is used in the exported types of @truffle/preserve-fs, e.g.:

import * as Preserve from "@truffle/preserve";
import { TargetPathOptions } from "./types";
export declare function targetPath(options: TargetPathOptions): Preserve.Process<Preserve.Target>;

If we set @truffle/preserve to a devDependency, I'm pretty sure that TypeScript will complain that it cannot find @truffle/preserve when you install @truffle/preserve-fs without also installing @truffle/preserve since it does not automatically get installed any more, but TypeScript does need it to make sense of the exported types.

@eggplantzzz
Copy link
Contributor

eggplantzzz commented Jun 15, 2021

@rkalis Is that true? All of the type info gets stripped out during compilation according to the docs. "It always gets fully erased" https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html

I guess we can test this.

@rkalis
Copy link
Contributor

rkalis commented Jun 15, 2021

I'm not 100% what the interaction will be in this case, but the snippet I pasted above is the compiled .d.ts output for @truffle/preserve-fs. So in a different project if you install @truffle/preserve-fs, TypeScript will look at those declaration files and not understand the Preserve.* types, since it can't find module @truffle/preserve. You're right that the compiled JS code doesn't care about any of the types, but if the consuming project uses TypeScript, I think it will fail compilation because it can't interpret the declaration file without having @truffle/preserve installed.

We can test the interaction to be sure.

@haltman-at
Copy link
Contributor Author

I mean, if you only need the dependency to compile, then it's a devDependency, right?

@rkalis
Copy link
Contributor

rkalis commented Jun 15, 2021

Yes, but I don't think the package's devDependencies get installed when installing a package, which means that type checking would fail when compiling a project that uses @truffle/preserve-fs. I might be wrong there though.

@gnidan
Copy link
Contributor

gnidan commented Jun 15, 2021

Let's play it safe and keep it a regular dep for now.

@haltman-at
Copy link
Contributor Author

OK, reverted that commit.

@haltman-at haltman-at merged commit 122549a into develop Jun 15, 2021
@haltman-at haltman-at deleted the import-type-initial branch June 15, 2021 22:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants