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

Complement vendor prefixed properties by compatibility data from MDN #27

Merged
merged 13 commits into from
Apr 11, 2018

Conversation

frenic
Copy link
Owner

@frenic frenic commented Apr 8, 2018

It's satisfying to see the amount of important properties this adds to the definitions. 🎉

EDIT
Question: The last line here removes vendor prefixed properties that's no longer supported. Without this line it will add a couple of dozen prefixed Moz and O properties like MozBoxShadow and OTransform. Do we want them or should they be removed?
Removed and deprecated properties are separated into a ObsoleteProperties interface marked as @deprecated. This enables warnings through TSLint and enables the possibility to manually exclude that interface.

Some tasks needs to be completed before this is done:

  • Reuse property type from original property (right now they are duplicated)
  • Add vendor prefixed keywords

@frenic frenic changed the title Complement vendor prefixes with compatibility data Complement vendor prefixed properties by compatibility data from MDN Apr 8, 2018
@pelotom
Copy link
Collaborator

pelotom commented Apr 8, 2018

Do we want them or should they be removed?

My vote would be to remove them if they're known to be no longer supported. Thus these typings can serve a valuable function of notifying people that they're using unsupported properties. It's important to remember that people can always choose to circumvent the type system with casts, but I would think 99% of users wouldn't want to be using deprecated properties.

@frenic
Copy link
Owner Author

frenic commented Apr 8, 2018

Yeah, that's my vote too. The only problem I can think of is releasing a patch release with updated MDN data where, lets say, a Webkit prefix for a new iOS version is suddenly removed and you get type errors in your project. But you want to keep the prefixed property over a certain period of time until end users has updated their iOS device.

At least there should be information about this in the README with examples of how to use module augmentation to extend interfaces with properties using TypeScript.

Not sure if something similar works with Flow though. But it could be possible to preserve them for Flow definitions not for TypeScript definitions.

@meyer
Copy link

meyer commented Apr 8, 2018

Perhaps you could leave them in but prepend a /** @deprecated */ docstring comment?

@pelotom
Copy link
Collaborator

pelotom commented Apr 8, 2018

Perhaps you could leave them in but prepend a /** @deprecated */ docstring comment?

That would be nice if it resulted in warnings for the users, but AFAIK it has no semantic meaning.

@pelotom
Copy link
Collaborator

pelotom commented Apr 8, 2018

At least there should be information about this in the README with examples of how to use module augmentation to extend interfaces with properties using TypeScript.

This seems like a good approach to me 👍

index.d.ts Outdated
@@ -189,17 +189,21 @@ export interface StandardLonghandProperties<TLength = string | 0> {
minInlineSize?: MinInlineSizeProperty<TLength>;
minWidth?: MinWidthProperty<TLength>;
mixBlendMode?: MixBlendModeProperty;
motionDistance?: MotionDistanceProperty<TLength>;
Copy link
Owner Author

@frenic frenic Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't feel like early drafts belongs in StandardProperties. They are not "standard" anymore. Should they be included in VendorProperties instead?

@frenic
Copy link
Owner Author

frenic commented Apr 8, 2018

That would be nice if it resulted in warnings for the users, but AFAIK it has no semantic meaning.

Well TSLint actually warns usage of identifiers marked with /** @deprecated */. So maybe that's a good idea.

@pelotom
Copy link
Collaborator

pelotom commented Apr 8, 2018

Oh interesting, I didn’t know about that!

@frenic frenic force-pushed the mdn-browser-compat branch 2 times, most recently from dbd1608 to ffb4b4d Compare April 9, 2018 13:05
@frenic
Copy link
Owner Author

frenic commented Apr 9, 2018

I separated the removed and deprecated properties into a ObsoleteProperties interface marked as @deprecated. In that way it enables warnings through TSLint and enables the possibility to manually exclude that interface.

csstype/index.d.ts

Lines 588 to 596 in ef757f2

export interface ObsoleteProperties<TLength = string | 0> {
/** @deprecated */
boxDirection?: BoxDirectionProperty;
/** @deprecated */
boxFlex?: GlobalsNumber;
/** @deprecated */
boxOrient?: BoxOrientProperty;
/** @deprecated */
boxPack?: BoxPackProperty;

@frenic frenic force-pushed the mdn-browser-compat branch 3 times, most recently from 1bc8215 to d06d03e Compare April 10, 2018 21:03
@frenic
Copy link
Owner Author

frenic commented Apr 10, 2018

I think I've got everything right now. I refactored some code so I hope I haven't introduced any bugs. I'll do some samples to verify.

@frenic frenic merged commit d62c2ab into master Apr 11, 2018
@frenic frenic deleted the mdn-browser-compat branch April 11, 2018 07:26
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