Skip to content

Remove const enum and add ErrorValue in index.d.ts #1317

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

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

aplum
Copy link
Contributor

@aplum aplum commented Jun 5, 2020

Summary

Using const enum in .d.ts files breaks compilation with --isolatedModules. Instead, normal enums have been used where a corresponding object exists at runtime. PaperSize does not exist at runtime, and so has been left as const enum for backwards compatibility.

This also adds a declaration for ErrorValue since it exists at runtime but had no TS definition. This does result in some duplication with CellErrorValue, but changing CellErrorValue to use ErrorValue would be a breaking change.

A comparable fix and explanation for the problem with using const enum can be found at chalk/chalk#296, and some further explanation at https://ncjamieson.com/dont-export-const-enums/ and this comment.

Test plan

I created a small project to test this while making changes. Example file:

import { ValueType, CellModel } from 'exceljs';

let vt: ValueType = ValueType.Date;
const vtString = ValueType.String;

export const cm1: Partial<CellModel> = { type: vt };
export const cm2: Partial<CellModel> = { type: vtString };
export const cm3: Partial<CellModel> = { type: ValueType.Boolean };

Before this PR:

  • No errors with isolatedModules: false
  • Errors with isolatedModules: true

With this PR:

  • No errors with isolatedModules: false
  • No errors with isolatedModules: true

Using `const enum` in .d.ts files breaks compilation with `--isolatedModules`. Instead, normal `enum`s have been used where a corresponding object exists at runtime. `PaperSize` does not exist at runtime, and so has been left as `const enum` for backwards compatibility.

This also adds a declaration for `ErrorValue` since it exists at runtime but had no TS definition. This does result in some duplication with `CellErrorValue`, but changing `CellErrorValue` to use `ErrorValue` would be a breaking change.
@alubbe alubbe requested a review from Siemienik June 8, 2020 09:11
@alubbe
Copy link
Member

alubbe commented Jun 8, 2020

@Siemienik do you think we could make the above typescript code into a test file and add it to our test suite?

@Siemienik
Copy link
Member

@aplum thanks 😄

@Siemienik Siemienik merged commit c0ead35 into exceljs:master Jun 8, 2020
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