-
Notifications
You must be signed in to change notification settings - Fork 294
improve typescript typings #330
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
base: master
Are you sure you want to change the base?
Conversation
Also renamed `scripts/template-index.d.ts` to `lib/index-template.d.ts`
Pull Request Test Coverage Report for Build 17037641521Details
💛 - Coveralls |
…reamingAPI`, `getCodec`, `_canonicalizeEncoding`, `encodings`, `_codecDataCache`, `defaultCharUnicode`, `defaultCharSingleByte` to the public API
…fs/improve-typings
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Hey, I just modified your PR to add tests for the types. I’m still testing a few things to make sure I don’t break anything, but feel free to push changes if you see something missing. |
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Removed semi-colons
I've pushed the fixes. I'm a bit concerned about the new dev dependencies and build configurations. One of the project's core values is "Simple/boring code... Minimum dependencies." This is a pure JavaScript library and the public API is very straightforward. This new infrastructure for typings seems to add more maintenance overhead than the value it provides for this specific project. Contributors should not have to fight types, or make redundant changes to multiple files to change the type definitions of the public API. Changing the implementation detail, function signature or even removing the function from the library will not fail the typescript tests. Only changes to |
It really does add value, having tests for the types is important. For example, TypeScript users report cases like this one: import-js/eslint-import-resolver-typescript#186. Changes in the types should be tested to avoid breaking things for these users |
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. | ||
* REQUIREMENT: This definition is dependent on the @types/node definition. | ||
* Install with `npm install @types/node --save-dev` | ||
*--------------------------------------------------------------------------------------------*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot remove Microsoft’s copyright unless Microsoft gives explicit approval for it to be removed (https://github.com/openjs-foundation/cross-project-council/blob/main/governance/IP_POLICY_GUIDANCE.md#2-copyright-notices).
Very valid. I've pushed the relevant changes. |
Improved type definitions when using this library.
npm run typegen
Options
toDecodeOptions
andEncodeOptions
I also noticed the "Testing" section in the README is out of date.