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

style(mixed): update typings for usage with UMD #1427

Closed
wants to merge 2 commits into from

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Mar 9, 2017

Fixes #1392.

@codecov-io
Copy link

codecov-io commented Mar 9, 2017

Codecov Report

Merging #1427 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1427   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files         141      141           
  Lines        2371     2371           
=======================================
  Hits         2365     2365           
  Misses          6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fd65da...4970490. Read the comment docs.

@layershifter
Copy link
Member Author

I've checked these changes with compiler and created local test files. It works, types are validated.

Seems, we have same case as in #1395 🐰 I would receive a review and feedback from a person more familiar with typescript of current changes and #1395.

@levithomason
Copy link
Member

Agreed on the extra reviewers here. I'm not sure about exporting a single namespace versus individual exports.

@levithomason
Copy link
Member

Perhaps @czb could test this out as he is requesting it. @dennari and @asvetliakov are also superb TS users and may have opinions here.

@layershifter
Copy link
Member Author

Needs rebase after #1467.

@levithomason
Copy link
Member

Fixed.

@layershifter
Copy link
Member Author

PR was merged, I've need make rebase and actualize this PR.

@layershifter
Copy link
Member Author

I want to close this for housekeeping. I don't see there any feedback from users while changes in this PR looks very ugly. I'm glad to see some feedback, possible it can be done more elegant.

@levithomason
Copy link
Member

Agreed, we can always revisit this but I would like to clean up the open issues and PR's as much as possible.

@layershifter layershifter deleted the chore/typings-cjs branch June 7, 2017 12:17
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.

3 participants