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

Manual patches pending MDN data updates #35

Closed
pelotom opened this issue Apr 12, 2018 · 6 comments · Fixed by #37
Closed

Manual patches pending MDN data updates #35

pelotom opened this issue Apr 12, 2018 · 6 comments · Fixed by #37

Comments

@pelotom
Copy link
Collaborator

pelotom commented Apr 12, 2018

Particularly now that React is using CSSType's Properties without a string index fallback, we're discovering more cases where the MDN data is missing properties and it's more pressing to have the typings be complete (sorry if I jumped the gun in creating this pressure!). It might be nice to have a release valve in the form of an interface of manual patches that we mix in to Properties, so we don't have to wait on MDN to merge PRs in order to be responsive to these missing properties. Then once the MDN PRs do get merged and the properties can be generated, we can delete them from the manual patches interface. This is essentially the approach taken with the SVG properties; would it make sense to generalize it?

Also if we take this approach I'm happy to volunteer to start creating PRs to patch the known missing properties!

@pelotom pelotom changed the title Manual patches pending MDN updates Manual patches pending MDN data updates Apr 12, 2018
@frenic
Copy link
Owner

frenic commented Apr 12, 2018

sorry if I jumped the gun in creating this pressure!

Haha, no problem. I was just about to start doing it my self when your PR showed up. So I was kind of expecting this to happen anyway. :)

It might be nice to have a release valve in the form of an interface of manual patches that we mix in to Properties, so we don't have to wait on MDN to merge PRs in order to be responsive to these missing properties.

Sounds quite reasonable. It could also be missing keywords or data types to existing properties and that could be troubling in some cases when merging two interfaces that share some properties. I would rather not use Exclude and that kind of stuff. My first though is to use something similar to what I did with SVG properties using a separate JSON file that's has similar schema. In that case it could look if a property existed in that "patching" sheet and merge the parsed syntax using a single bar combinator.

// MDN syntax
<color> || bold || <length>

// Patch syntax
light | <box>

// MDN and patch syntax
[ <color> || bold || <length> ] | light | <box>

More advanced than this isn't needed because they will resolve to string anyway. With this solution we could also output some build information in the terminal when the patch can be safely removed.

@pelotom
Copy link
Collaborator Author

pelotom commented Apr 12, 2018

I would rather not use Exclude and that kind of stuff.

Agreed, we shouldn't narrow the range of supported TS versions unnecessarily.

With this solution we could also output some build information in the terminal when the patch can be safely removed.

I hadn't thought of that, that's a great point!

@frenic frenic mentioned this issue Apr 13, 2018
2 tasks
@frenic
Copy link
Owner

frenic commented Apr 13, 2018

I've started a WIP pull request for this. I've a question there:

What if someone patch a property but MDN rejects to change their data for some reason? I consider these patches highly temporarily.

Should we introduce some kind of prerelease for this? That the stable contains typings from MDN data only (except for the SVG for now) and the prereleases contains typings from MDN data and patches?

@pelotom
Copy link
Collaborator Author

pelotom commented Apr 13, 2018

Should we introduce some kind of prerelease for this? That the stable contains typings from MDN data only (except for the SVG for now) and the prereleases contains typings from MDN data and patches?

So then there would be a prerelease branch that had to continually merge from master? I'm not convinced this is worth the effort. How often do you think it would happen that MDN rejects changes? We're only talking about changes that are officially part of the spec, just haven't made it into the MDN data repo yet, right?

In the unlikely event that such a thing happened though, I'd advocate first marking the property /** @deprecated */ for a period of time, and then possibly deleting.

I think putting these things on a prerelase would also be a major headache for downstream users who want these patch properties (which is, I think, everyone?). Consider: most users aren't depending directly on csstype, but using it indirectly through @types/react or some other major library. They probably don't want to manage the csstype dependency themselves, they just want to get the latest typings whenever they install the intermediate library. Unless the prerelease was marked as the "latest", they wouldn't get that by default. And then consider, who would actually want the non-patched version? I don't think there's a market for it.

So my feeling is there should just be a single stream of versions: the latest combination of autogenerated and patched properties. Over time, some properties will be marked deprecated and then maybe removed, and that's fine.

@frenic
Copy link
Owner

frenic commented Apr 13, 2018

And then consider, who would actually want the non-patched version? I don't think there's a market for it.

Yeah, you're probably right. The idea just popped up in my head with no major reflection.

We're only talking about changes that are officially part of the spec, just haven't made it into the MDN data repo yet, right?

I though so too. But then @wbamberg posted this comment to one of my PRs. It would be really interesting to hear what the policy is. Because we both have cases with old specs and some really old ones too. Like vendor properties that doesn't even have any official spec but works great in some versions.

It's just that I don't know 100% how to handle issues like #41 and #39 at the moment and how MDN would react on adding them. Adding properties as a patch and then remove them because MDN rejected it doesn't sound trivial either. It will become very frustrating for all of our end-users.

@pelotom
Copy link
Collaborator Author

pelotom commented Apr 13, 2018

I think we should aim to only patch things that are officially accepted parts of the spec, which MDN data is just temporarily missing for whatever reason. As noted in that comment, background-clip doesn't seem to fall in that category because it's only a draft specification. I think it's fine to tell users to use module augmentation for properties like that.

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 a pull request may close this issue.

2 participants