Skip to content

Conversation

plbstl
Copy link

@plbstl plbstl commented Aug 14, 2025

Improved type definitions when using this library.

  • Updated typedefs for the public API
  • Added exhaustive typedefs for all supported encodings.
  • Added a script to auto generate typings from source code: npm run typegen
  • Separated encoding Options to DecodeOptions and EncodeOptions

I also noticed the "Testing" section in the README is out of date.

# To view performance:
-- node test/performance.js
++ node performance/index.js

# To view test coverage:
-- npm run test:coverage
++ npm run test:cov

@coveralls
Copy link

coveralls commented Aug 14, 2025

Pull Request Test Coverage Report for Build 17037641521

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.837%

Totals Coverage Status
Change from base Build 17015959667: 0.0%
Covered Lines: 944
Relevant Lines: 1006

💛 - Coveralls

@bjohansebas
Copy link
Member

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>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
@plbstl
Copy link
Author

plbstl commented Aug 17, 2025

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 lib/index.d.ts are relevant to testing typescript. Seems kinda redundant to have these typescript tests.

@bjohansebas
Copy link
Member

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

Comment on lines 1 to 6
/*---------------------------------------------------------------------------------------------
* 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`
*--------------------------------------------------------------------------------------------*/
Copy link
Member

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).

@plbstl
Copy link
Author

plbstl commented Aug 18, 2025

Very valid. I've pushed the relevant changes.

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