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

[api change] Make additionalProperties a Record #65

Merged

Conversation

scoiatael
Copy link
Collaborator

Sorry I didn't make this work when introducing this feature, but I didn't know enough TypeScript. Anyhow, Record type is better here, since it makes checking keys much more efficient.

Also, a couple of additional quality-of-life-changes:

  • expose AliKey as part of public API,
  • expose additionalProperties on Group (things like ArtboardData are on Group, not Layer).

Let me know WDYT :)

Sorry I didn't make this change when introducing this feature, but I
didn't know enough TypeScript to make it work. Anyhow, Record type is
better here, since it makes checking keys much more efficient.

Also, a couple of additional quality-of-life-changes:
- expose `AliKey` as part of public API,
- expose `additionalProperties` on Group (things like `ArtboardData` are
on Group, not Layer).
@scoiatael
Copy link
Collaborator Author

@pastelmind @dlehdanakf WDYT? Looks like CI passes now. Should I assign one of you (which one) as a reviewer? :)

@pastelmind
Copy link
Collaborator

pastelmind commented Nov 9, 2022

That is odd. Object.fromEntries() is available in Node 14 according to node.green. Maybe there is a problem with our ESLint or TypeScript config?

Edit: Looks like our browserslist config specifies minimum Node.js version as 12.0.0:

psd/package.json

Lines 19 to 25 in 5e1fd35

"browserslist": [
"chrome >= 57",
"firefox >= 52",
"safari >= 11",
"edge >= 79",
"node >= 12"
],

Object.fromEntries() should be available in Node.js v12.0.0, according to MDN, this V8 blog article, and this tweet. So the compilerOptions.lib setting in our tsconfig.base.json does not quite accurately reflect the landscape of our targets.

But some of our browser targets do not support Object.fromEntries(), yet. To use it without a polyfill, we would have to bump our minimum supported browser version to higher values.

In summary, the Object.fromEntries() polyfill is a welcome addition, but not because of Node.js, but rather the browsers we intend to support.

@pastelmind pastelmind added the type: enhancement New feature or request label Nov 12, 2022
@pastelmind pastelmind merged commit 07fdcc8 into webtoon:main Nov 14, 2022
@scoiatael scoiatael deleted the feature/additionalPropertiesTypes branch November 14, 2022 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants