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

Types for processRow option of RollupDSV #1492

Closed
jamesscottbrown opened this issue May 12, 2023 · 2 comments
Closed

Types for processRow option of RollupDSV #1492

jamesscottbrown opened this issue May 12, 2023 · 2 comments

Comments

@jamesscottbrown
Copy link

  • Rollup Plugin Name: DSV
  • Rollup Plugin Version: 3.0.2

Expected Behavior / Situation

The README for the DSV plugin sats that the processRow function can "either manipulate the passed row, or return an entirely new row object."

I would expect that:

  • manipulating the passed row and returning nothing should not be a type error
  • returning a new row object that includes numbers should not be a type error (one of the main purposes of using the function is to convert string values to Number or Date values)

Actual Behavior / Situation

The type definition for processRow is:

  processRow?: null | ((row: DSVRowString, id: string) => DSVRowString | undefined);

Problem 1: there is an example in the README, but it gives a type error because the function doesn't return anything, so has a return type of void rather than undefined:

dsv({
  processRow: (row, id) => {
    Object.keys(row).forEach((key) => {
      var value = row[key];
      row[key] = isNaN(+value) ? value : +value;
    });
  }
});

Problem 2: constructing a new row object gives a type error, because processRow is expected to return a value of type DSVRowString, which has the type definition

export interface DSVRowString {
    [key: string]: string;
}

Modification Proposal

Change the processRow type definition to something like:

  processRow?: null | ((row: DSVRowString, id: string) => DSVRowAny | undefined | void);
@shellscape
Copy link
Collaborator

Seems reasonable. Please feel free to open a PR

@stale
Copy link

stale bot commented Aug 12, 2023

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants