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 import types to refer to declarations in declaration emit #24071

Merged

Conversation

weswigham
Copy link
Member

With this change, we consider all exported members as 'visible' for declaration emit, and then use import types to reference them. This means, eg, this:

// @filename: file1.ts
export class Foo {}
// @filename: file2.ts
export function foo(): import("./file1").Foo {
    return null as any;
}
// @filename: file3.ts
import {foo} from "./file2";
export function bar() {
    return foo();
}

emits these declarations:

// @filename: file1.d.ts
export class Foo {}
// @filename: file2.d.ts
export function foo(): import("./file1").Foo;
// @filename: file3.d.ts
export function bar(): import("./file1").Foo;

Small todo/discussion point: Based on discussions we've had internally, if there are other import statements preserved in the declaration emit (or maybe all the time? IDK - I personally prefer the import types in many cases), we should coalesce repeated imports into a single synthetic import statement - this is just a style adjustment, as it would just cut down on repeated import types in favor of import x = require("") declarations. However, from a "makes the declaration authoring experience better" PoV, this is already usable as is. It'd be nice to know what people's preferences are here, though.

Fixes #9944
Invalidates #22132

if (some(symbol.declarations, hasExternalModuleSymbol)) {
// Any meaning of a module symbol is always accessible via an `import` type
return {
accessibility: SymbolAccessibility.Accessible
Copy link
Contributor

@mhegazy mhegazy May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a set of error messages that should be unused now, e.g. * from external module blah and can not be named errors, we should remove these..

Copy link
Member Author

@weswigham weswigham May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically those are still possible:

// file1:
export function f() {
  interface Local { x: any }
  const y: Local = null as any;
  return y;
}
// file2:
import { f } from "./file1"
export default f();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see..

@RomanHotsiy
Copy link

Thanks, @weswigham to finally fixing this issue 🎉 !
Is there any chance it can land into the upcoming 2.9 release?

@weswigham weswigham merged commit 3fc727b into microsoft:master May 17, 2018
@weswigham weswigham deleted the import-types-of-different-stripes branch May 17, 2018 20:08
@simonbuchan
Copy link

Don't think I didn't see that branch name...

weswigham added a commit that referenced this pull request May 17, 2018
* Optimize intersections of unions of unit types

* Add regression test

* Accept new baselines

* LEGO: check in for master to temporary branch.

* Add Unicode ThirdPartyNotice

* Properly handle edge cases

* Sort the whole diagnostic, plus giving up on isolating tests (#24186)

* Sort the whole diagnostic

* Also strip references to our repos node_modules, since removing it is hard

* LEGO: check in for master to temporary branch.

* add quick fix for import type missing typeof

* Add callback tag, with type parameters (#23947)

* Add initial tests

* Add types

* Half of parsing (builds but does not pass tests)

* Parsing done; types are uglier; doesn't crash but doesn't pass

* Bind callback tag

Builds but tests still don't pass

* Only bind param tags inside callback tags

* Fix binding switch to only handle param tags once

* Checking is 1/3 done or so.

Now I'm going to go rename some members to be more uniform. I hate
unnnecessary conditionals.

* Rename typeExpression to type (for some jsdoc)

(maybe I'll rename more later)

* Rename the rest of typeExpressions

Turns out there is a constraint in services such that they all need to
be named the same.

* Few more checker changes

* Revert "Rename the rest of typeExpressions"

This reverts commit f41a96b.

* Revert "Rename typeExpression to type (for some jsdoc)"

This reverts commit 7d2233a.

* Finish undoing typeExpression rename

* Rename and improve getTypeParametersForAliasSymbol

Plus some other small fixes

* Core checking works, but is flabbergastingly messy

I'm serious.

* Callback return types work now

* Fix crash in services

* Make github diff smaller

* Try to make github diff even smaller

* Fix rename for callback tag

* Fix nav bar for callback tag

Also clean up some now-redundant code there to find the name of typedefs.

* Handle ooorder callback tags

Also get rid of redundant typedef name code *in the binder*. It's
everywhere!

* Add ooorder callback tag test

* Parse comments for typedef/callback+display param comments

* Always export callbacks

This requires almost no new code since it is basically the same as
typedefs

* Update baselines

* Fix support for nested namespaced callbacks

And add test

* Callbacks support type parameters

1. Haven't run it with all tests
2. Haven't tested typedef tags yet
3. Still allows shared symbols when on function or class declarations.

* Template tags are now bound correctly

* Test oorder template tags

It works.

* Parser cleanup

* Cleanup types and utilities

As much as possible, and not as much as I would like.

* Handle callback more often in services

* Cleanup of binder and checker

* More checker cleanup

* Remove TODOs and one more cleanup

* Support parameter-less callback tags

* Remove extra bind call on template type parameters

* Bind template tag containers

Doesn't quite work with typedefs, but that's because it's now stricter,
without the typedef fixes. I'm going to merge with jsdoc/callback and
see how it goes.

* Fix fourslash failures

* Stop pre-binding js type aliases

Next up, stop pre-binding js type parameters

* Further cleanup of delayed js type alias binding

* Stop prebinding template tags too

This gets rid of prebinding entirely

* Remove TODO

* Fix lint

* Finish merge with use-jsdoc-aliases

* Update callback tag baselines

* Rename getTypeParametersForAliasSymbol

The real fix is *probably* to rename Type.aliasTypeArguments to
aliasTypeParameters, but I want to make sure and then put it in a
separate PR.

* moveToNewFile: Don't move imports (#24177)

* Reduce map lookups (#24203)

* Fix jsdoc type resolution [merge to master] (#24204)

* Fix JSDoc type resolution

Breaks type parameter resolution that is looked up through prototype
methods, though. I need to fix that still.

* Check for prototype method assignments first

* Undo dedupe changes to getJSDocTags

* JS Type aliases can't refer to host type params

Previously, js type aliases (@typedef and @callback) could refer to
type paremeters defined in @template tags in a *different* jsdoc tag, as
long as both tags were hosted on the same signature.

* Reduce dedupe changes+update baseline

The only reason I had undone them was to merge successfully with an
older state of master.

* Add undefined guard

* moveToNewFile: Fix bug for VariableDeclaration missing initializer (#24214)

* Use import types to refer to declarations in declaration emit (#24071)

* Stand up a simple implementation using import types for exports of modules which are otherwise inaccessible

* Ensure references exist to link to modules containing utilized ambient modules

* Accept baselines with new import type usage

* Fix lint

* Accept changed baseline (#24222)

* fixUnusedIdentifier: Don't delete node whose ancestor was already deleted (#24207)

* More robust circularity detection in node builder (#24225)

* Improve ChangeTracker#deleteNodeInList (#24221)

* moveToNewFile: Fix bug for missing importClause (#24224)

* Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right (#24184)

* Set startPos at EOF in jsdoc token scanner to node end positions for nodes terminated at EoF are right

* More complete nonwhitespace token check, fix syntactica jsdoc classifier

* Use loop and no nested lookahead

* Do thigns unrelated to the bug in the test

* Fix typo move return

* Patch up typedef end pos

* Fix indentation, make end pos target more obvious
@n8agrin
Copy link

n8agrin commented Jul 5, 2018

I may not be understanding a subtlety of what this PR is attempting to solve, but I do think it was supposed to solve a problem I'm still running into when building React components in Typescript, specifically the TS4023 error Exported variable 'Button' has or is using name 'React.ClassAttributes' from external module "<PATH>/node_modules/@types/react/index" but cannot be named. This is with the following package.json, tsconfig.json file and from running tsc:

package.json

 ...
  "devDependencies": {
    "@babel/cli": "7.0.0-beta.44",
    "@babel/core": "7.0.0-beta.44",
    "@babel/plugin-proposal-class-properties": "7.0.0-beta.44",
    "@babel/plugin-proposal-object-rest-spread": "7.0.0-beta.44",
    "@babel/plugin-transform-runtime": "7.0.0-beta.44",
    "@babel/preset-env": "7.0.0-beta.44",
    "@babel/preset-react": "7.0.0-beta.44",
    "@babel/preset-typescript": "7.0.0-beta.44",
    "@types/classnames": "2.2.3",
    "@types/jest": "22.2.2",
    "@types/react": "16.4.2",
    "@types/react-dom": "16.0.6",
    "@types/react-intl": "2.3.5",
    "@types/react-test-renderer": "16.0.1",
    "babel-jest": "22.4.3",
    "babel-plugin-looker-inline-sass": "1.0.4",
    "babel-preset-env": "1.6.1",
    "babel-preset-react": "6.24.1",
    "css-loader": "0.28.11",
    "identity-obj-proxy": "3.0.0",
    "jest": "22.4.3",
    "node-sass": "4.8.3",
    "prop-types": "15.6.1",
    "react-docgen-typescript": "1.2.6",
    "react-styleguidist": "7.0.5",
    "react-test-renderer": "16.3.1",
    "regenerator-runtime": "0.11.1",
    "request": "2.85.0",
    "rimraf": "2.6.2",
    "sass-loader": "6.0.7",
    "styled-components": "3.3.3",
    "stylish": "1.0.0",
    "ts-jest": "22.4.2",
    "ts-loader": "3.5.0",
    "tslint": "5.10.0",
    "tslint-config-standard": "7.1.0",
    "tslint-react": "3.6.0",
    "typescript": "2.9.2",
    "typings-for-css-modules-loader": "1.7.0",
    "webpack": "3.5.0"
  },
  "dependencies": {
    "@babel/runtime": "7.0.0-beta.44",
    "chroma-js": "^1.3.7",
    "classnames": "2.2.5",
    "insert-css": "2.0.0",
    "polished": "1.9.3",
    "react": "16.4.1",
    "react-dom": "16.4.1",
    "svg-inline-loader": "^0.8.0"
  },
  "peerDependencies": {
    "styled-components": "3.3.3"
  }
 ...

tsconfig.json

{
  "compilerOptions": {
    "allowSyntheticDefaultImports": true,
    "declaration": true,
    "jsx": "react",
    "noFallthroughCasesInSwitch": true,
    "noImplicitReturns": true,
    "noUnusedParameters": true,
    "outDir": "dist",
    "plugins": [
      {
        "name": "tslint-language-service",
        "alwaysShowRuleFailuresAsWarnings": true
      }
    ],
    "skipLibCheck": true,
    "sourceMap": true,
    "strict": true,
    "module": "commonjs",
    "target": "es6"
  },
  "include": [
    "./src/**/*.ts",
    "./src/**/*.tsx",
    "./config/typescript/**/*"
  ],
  "exclude": [
    "./src/**/*.test.ts",
    "./src/**/*.test.tsx",
  ]
}

I'm trying to create a library of React components for other Typescript projects to consume so I must generate declaration files. A simple repro of this error should occur when trying to compile this React component created by StyledComponents:

import styled from 'styled-components'
export interface ButtonProps {}
export const Button = styled<ButtonProps, 'button'>('button')``

A "fix" for the compiler error which also makes tslint and the tsconfig noUnusedLocals setting happy is:

import styled, {StyledComponentClass} from 'styled-components'
import * as React from 'react
export { React, StyledComponentClass }

export interface ButtonProps {}
export const Button = styled<ButtonProps, 'button'>('button')``

Adding the import * as React from 'react' and import { StyledComponentClass} from 'styled-components' statements is non-obvious. Confusingly, tsc can compile the code to working Javascript fine without those imports. Omitting them only produces an error when declaration files are being emitted, when the library is being packaged for distribution. This dichotomy makes this error extremely confusing and non-obvious.

Is there something I'm missing here? StyleComponents itself imports the React types. I am surprised the Typescript compiler can't walk the dependency tree and extract the exported types it needs to build declaration files.

@weswigham
Copy link
Member Author

@n8agrin You're probably looking at an instance of #24666.

@n8agrin
Copy link

n8agrin commented Jul 11, 2018

@weswigham 👍 thank you for the response and your work!

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.

5 participants