Use `import type` for types instead of `import` #607

DeepDoge · 2022-01-20T11:54:18Z

Made it generate type imports with import type instead of import alone.

What I changed:

Before createImportDeclaration and createImportTypeDeclaration functions were both doing the exact same thing, now createImportTypeDeclaration imports with import type.
When createImportsForUsedIdentifiers used, if the moduleSpecifier is starting with type, it's using createImportTypeDeclaration for it instead of createImportDeclaration.

I did this because this is how it's suppose to be and it was causing problems with Svelte's Vite.

- importing types without 'import type' was problamatic with svelte vite

krzkaczor

Thanks for this PR! 🙇

Personally, I don't use import/export type that much but I get that some tooling might require it. The problem with this PR is that right now our tests can't catch any mistakes related to this. I can change import to import type and the other way around and CI still passes.

How can we verify that current changes work?

krzkaczor · 2022-01-29T16:53:22Z

packages/target-ethers-v5/src/codegen/index.ts

-        'BigNumberish',
-      ],
-      '@ethersproject/providers': ['Provider', 'TransactionRequest'],
+      ethers: ['Signer', 'utils', 'Contract', 'ContractFactory', 'PayableOverrides', 'BytesLike', 'BigNumberish'],


I can change this line to:

Suggested change

ethers: ['Signer', 'utils', 'Contract', 'ContractFactory', 'PayableOverrides', 'BytesLike', 'BigNumberish'],

'type ethers': ['Signer', 'utils', 'Contract', 'ContractFactory', 'PayableOverrides', 'BytesLike', 'BigNumberish'],

(merge with next line) and all tests still pass.

DeepDoge · 2022-01-29T20:07:33Z

Do you also want me to edit tests so when something that is a type imported without import type or the other way around, we catch it in the tests?

changeset-bot · 2022-01-29T22:47:56Z

⚠️ No Changeset found

Latest commit: a8554a4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

DeepDoge · 2022-01-29T23:01:51Z

@krzkaczor

Added "importsNotUsedAsValues": "error" in tsconfig in the root.
- This should make the typecheck fail if there is any type imported without import type and haven't been used as value inside the file.
And also updated all of the files in the project accordingly.

krzkaczor · 2022-01-30T11:25:45Z

scripts/.eslintrc.js

@@ -0,0 +1,6 @@
+/** @type {import("eslint").Linter.Config} */


Is this file needed?

I realized that it was giving ide errors because the folder had files with console.logs but the root eslint config doesn't allow console.logs.

But scripts folder contains files for cli scripts.

So i suppose it's needed.

Hmm but this way this file is also not used by anything.... I think the real solution would be to enforce root eslint config for every single file (right now it's only for code that is actually published) and then ignore valid console.logs with eslint ignore comments. I'd remove this from this PR and do this in a separate one.

krzkaczor · 2022-01-30T11:27:04Z

tsconfig.json

@@ -9,7 +9,8 @@
    "sourceMap": true,
    "composite": true,
    "incremental": true,
-    "exactOptionalPropertyTypes": true
+    "exactOptionalPropertyTypes": true,
+    "importsNotUsedAsValues": "error"


what if we enforce this only in packages/target-ethers-v5/tsconfig.json? I don't feel that it's necessary to use accross the whole codebase.

krzkaczor · 2022-01-30T11:28:25Z

@DeepDoge thanks! This flag importsNotUsedAsValues is what we need (i think). It just should be scoped to ethers test package. I hope there is automatic way to fix it 😅

DeepDoge · 2022-01-30T12:08:22Z

@krzkaczor I mean, I believe it would be good that if we use it for the whole codebase. It's more consistent and I already updated the whole codebase for it.
Why it would be good because for example while im running the tests, i also realized that other than ethers v5, ethers v4 and web3 had the same problem too. And I fixed those too.
I didn't realized those before because i was only using ethers5 in my project.

But if you really don't want it for the whole codebase, I can only scope it for, ethers v5, v4 and web3 tests.

EDIT: Left for my military service, brb next month.

krzkaczor · 2022-02-03T10:33:54Z

But if you really don't want it for the whole codebase, I can only scope it for, ethers v5, v4 and web3 tests.

Yes, I would prefer to do it like this. I am not sure if there is an automatic way to enforce proper use of import type and I don't want to put the additional burden on maintainers/contributors.

EDIT: Left for my military service, brb next month.

Good luck! :)

DeepDoge · 2022-03-07T11:22:01Z

@krzkaczor can you check now?

hasparus · 2022-03-07T20:36:23Z

packages/hardhat-test/test/hardhat.test.ts

@@ -20,8 +20,12 @@ describe('TypeChain x Hardhat', () => {
    const artifact = require('../artifacts/contracts/Counter.sol/Counter.json') as { abi: Interface; bytecode: string }

    const counterFactory1 = new Counter__factory()
+    counterFactory1


Why is this needed?

krzkaczor · 2022-03-07T21:07:28Z

Superseded by #636

krzkaczor · 2022-03-07T22:07:51Z

Thanks, @DeepDoge for your work on this 🙇 I merged it as #636.

DeepDoge added 2 commits January 20, 2022 11:13

Use 'import type' for types instead of 'import'

b162846

- importing types without 'import type' was problamatic with svelte vite

Ran test:fix after changes to use 'import type' for types

bb0e63d

sebastinez mentioned this pull request Jan 21, 2022

Audit fixes and improved anchors radicle-dev/radicle-explorer#165

Closed

krzkaczor requested changes Jan 29, 2022

View reviewed changes

DeepDoge added 4 commits January 29, 2022 22:02

import type fixes

889fc64

import type fixes

1040ddd

import type fixes

74bb72f

import type fixes, before push

11a3680

krzkaczor requested changes Jan 30, 2022

View reviewed changes

DeepDoge added 3 commits March 7, 2022 12:04

Merge branch 'dethcrypto:master' into master

8067407

import type checks only scoped for tests

0ef2f78

Removed unrelated file from PR

a8554a4

hasparus reviewed Mar 7, 2022

View reviewed changes

krzkaczor closed this Mar 7, 2022

krzkaczor mentioned this pull request Mar 7, 2022

Use 'import type' for types instead of 'import' #636

Merged

	ethers: ['Signer', 'utils', 'Contract', 'ContractFactory', 'PayableOverrides', 'BytesLike', 'BigNumberish'],
	'type ethers': ['Signer', 'utils', 'Contract', 'ContractFactory', 'PayableOverrides', 'BytesLike', 'BigNumberish'],

		@@ -0,0 +1,6 @@
		/** @type {import("eslint").Linter.Config} */

Use import type for types instead of import #607

Use import type for types instead of import #607

Uh oh!

Conversation

DeepDoge commented Jan 20, 2022

Uh oh!

krzkaczor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krzkaczor Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

DeepDoge commented Jan 29, 2022

Uh oh!

changeset-bot bot commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

DeepDoge commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krzkaczor Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

DeepDoge Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

krzkaczor Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

krzkaczor Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

krzkaczor commented Jan 30, 2022

Uh oh!

DeepDoge commented Jan 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krzkaczor commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDoge commented Mar 7, 2022

Uh oh!

hasparus Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

krzkaczor commented Mar 7, 2022

Uh oh!

krzkaczor commented Mar 7, 2022

Uh oh!

Uh oh!

Use `import type` for types instead of `import` #607

Use `import type` for types instead of `import` #607

krzkaczor left a comment •

edited

Loading

changeset-bot bot commented Jan 29, 2022 •

edited

Loading

DeepDoge commented Jan 29, 2022 •

edited

Loading

DeepDoge commented Jan 30, 2022 •

edited

Loading

krzkaczor commented Feb 3, 2022 •

edited

Loading