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

Defer to rollup TS plugin to build types #3564

Merged
merged 4 commits into from
Sep 6, 2019

Conversation

jednano
Copy link
Contributor

@jednano jednano commented Sep 5, 2019

Same as cellog#1

@timdorr
Copy link
Member

timdorr commented Sep 5, 2019

I don't want to name the index file something other than index. That's a Node standard I would like to keep. Please revert it back to index.d.ts.

@jednano
Copy link
Contributor Author

jednano commented Sep 5, 2019

index.js is fine, so long as there are types in index.d.ts.

@jednano
Copy link
Contributor Author

jednano commented Sep 5, 2019

I think the only way to make you happy is to do some manual renaming after the rollup build.

@jednano
Copy link
Contributor Author

jednano commented Sep 5, 2019

@timdorr fixed in 17fa65a. You'll notice/hate that there are 3 move-file commands in the npm run build script. This is necessary to ensure the d.ts files are renamed according to the rollup configuration.

package.json Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@ import {
Dispatch,
bindActionCreators,
ActionCreatorsMapObject
} from 'redux'
} from '../..'
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. This could have caused a number of problems for people working on the code!

This also puts them back into the types path. However, I don't know how to have them output as a single file bundle to match our other outputs.
@timdorr
Copy link
Member

timdorr commented Sep 6, 2019

OK, made a change to only put out the declarations once, as they were showing up in each of the output types. Here's what things look like now:

 ⇶ ls es dist lib types
dist:
redux.js  redux.min.js

es:
redux.js  redux.mjs

lib:
redux.js

types:
applyMiddleware.d.ts  bindActionCreators.d.ts  combineReducers.d.ts  compose.d.ts  createStore.d.ts  index.d.ts  types/  utils/

I don't know how to get the declarations to be flat like the other output bundles, or if that's even really possible. I don't think it's super-necessary, so this is looking good as-is. Thanks for following up on this @jedmao!

@timdorr timdorr merged commit d8417e8 into reduxjs:ts-conversion Sep 6, 2019
@jednano
Copy link
Contributor Author

jednano commented Sep 6, 2019

Just so you know, this fixes duplication, but it still breaks direct imports or imports that go deeper than "redux".

e.g., import {} from 'redux/redux' or from 'redux/foo' if you care at all to support those types of import statements and preserve types.

@timdorr
Copy link
Member

timdorr commented Sep 6, 2019

We haven't supported direct imports since moving to flat bundles.

@jednano jednano deleted the rollup-types branch September 6, 2019 17:17
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* Defer to rollup TS plugin to build types

* index.ts -> redux.js, redux.d.ts

* Only output one copy of the types.

This also puts them back into the types path. However, I don't know how to have them output as a single file bundle to match our other outputs.

* Remove the move


Co-authored-by: Tim Dorr <git@timdorr.com>
Former-commit-id: 75c882b
Former-commit-id: aaa10aa
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* Defer to rollup TS plugin to build types

* index.ts -> redux.js, redux.d.ts

* Only output one copy of the types.

This also puts them back into the types path. However, I don't know how to have them output as a single file bundle to match our other outputs.

* Remove the move


Co-authored-by: Tim Dorr <git@timdorr.com>
Former-commit-id: 75c882b
Former-commit-id: aaa10aa
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

2 participants