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

fully move away from index.d.ts #3561

Merged

Conversation

cellog
Copy link
Contributor

@cellog cellog commented Sep 3, 2019

this is it! after this, typescript conversion is complete

package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@timdorr
Copy link
Member

timdorr commented Sep 3, 2019

Also, we should add the types path to the gitignore and remove those generated files from the repo.

tsconfig.json Show resolved Hide resolved
tsconfig.json Outdated
// "declaration": true /* Generates corresponding '.d.ts' file. */,
// "declarationMap": true /* Generates a sourcemap for each corresponding '.d.ts' file. */,
"declaration": true /* Generates corresponding '.d.ts' file. */,
"declarationMap": true /* Generates a sourcemap for each corresponding '.d.ts' file. */,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

question: is this useful? It pops a sourcemap into the types/ directory but I'm not sure anyone would use it?

Copy link
Member

Choose a reason for hiding this comment

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

Does VS Code pick this up when I hit Go To Definition? That's what I would assume to use it for.

I'm inclined to thing they're not useful, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another question: Do we still need the .d.ts files? Can't we just distribute src/ now?

Copy link
Member

Choose a reason for hiding this comment

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

Not the individual ones, just the main index.d.ts. Rewinding back a bit, I would expect that to be in the package root like it is now.

We already put the src into the package verbatim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so is there any reason we can't do "typings": "src/index.ts"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the individual ones, just the main index.d.ts. Rewinding back a bit, I would expect that to be in the package root like it is now.

I tried to do this, it isn't possible in an automatic way, because typescript automatically ignores the directory we tell it to pop its results into, probably to avoid infinite loops.

We can just manually cp types/index.d.ts . but it seems unnecessary to me. I haven't seen any other examples out there that keep any autogenerated files in the repo

Also, I checked, and no one is using "types": "index.ts" so I'm going to assume it's a bad idea

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, we don't need definitions because we are fully TS'ed now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@timdorr "Go to Definition" doesn't work like that with sourcemaps.

@cellog "types": "index.ts" is definitely a bad idea, as the types are supposed to point to a .d.ts file. All TS files should be gone when you publish to npm.

@timdorr do you mean we don't need manually-typed definitions? Correct. Still need auto-generated ones though.

Copy link
Member

Choose a reason for hiding this comment

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

We publish src now. We want that in the package so the source is always available to read through. It's small and I consider it an important feature of Redux.

Whether we use that for our types or not, it's going to stay in the package.

package.json Outdated Show resolved Hide resolved
@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2019

I think we are ready? @timdorr anything I missed?

@cellog cellog force-pushed the cellog-3500-generate-index.d.ts branch from b54a776 to 291c263 Compare September 3, 2019 19:11
@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2019

ok, just rebased this, when it's done, should be good to go

@jednano
Copy link
Contributor

jednano commented Sep 5, 2019

@cellog there is some type duplication in this PR that I fixed in cellog#1

@timdorr timdorr merged commit 766f934 into reduxjs:ts-conversion Sep 5, 2019
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* fully move away from index.d.ts

* build types into the types/ directory

* remove autogenerated types dir

* ignore auto-generated types

* better type building, also clean old definitions

* use types instead of typings

Co-Authored-By: Jed Mao <jedmao@users.noreply.github.com>

* Don't build declaration maps

Former-commit-id: 20c995c
Former-commit-id: cc5bb53
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* fully move away from index.d.ts

* build types into the types/ directory

* remove autogenerated types dir

* ignore auto-generated types

* better type building, also clean old definitions

* use types instead of typings

Co-Authored-By: Jed Mao <jedmao@users.noreply.github.com>

* Don't build declaration maps

Former-commit-id: 20c995c
Former-commit-id: cc5bb53
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.

None yet

3 participants