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

Use MDN to enhance CSS LS. Part of #68 #91

Merged
merged 10 commits into from
Apr 23, 2018
Merged

Use MDN to enhance CSS LS. Part of #68 #91

merged 10 commits into from
Apr 23, 2018

Conversation

octref
Copy link
Contributor

@octref octref commented Apr 16, 2018

This PR:

  • Adds dependency on mdn-data (can we switch to yarn.lock instead of package-lock.json entirely?)
  • Generate browsers.ts by combining VSC and MDN data, while giving preference to VSC data
  • Show syntax and status in completion item's documentation.

Questions:

  • How should we generate restrictions / values? Currently for any of the new properties that MDN data introduces, we have no restriction and the value auto completion is inaccurate.
  • Our data contains 239 properties that's not in MDN Data. However, a majority of them are actually SVG properties (tracked in Add SVG data mdn/data#59). We can either
    • Hand curate the 239 properties to tell which ones are not SVG related, and mark those as deprecated / obsolete.
    • Wait for Add SVG data mdn/data#59

@octref octref requested a review from aeschli April 16, 2018 23:19
@aeschli
Copy link
Contributor

aeschli commented Apr 18, 2018

Note thatbrowsers.js should be as small as possible. vscode-css-languageservice is also used in the browser (for monaco) and we want the minimal memory footprint as well as download package size.
So browsers.js should only keep the properties that are used in the code.

@aeschli
Copy link
Contributor

aeschli commented Apr 18, 2018

I like the use of status. Maybe we should omit "status": "standard" in order to make browser.js smaller.

@chriseppstein
Copy link

So, mdn-data is licensed as MPL which is a somewhat copyleft license. This created some major headaches for me when I needed to have a dependency on it and I suspect it may have issues at other companies as well. Maybe at Microsoft if they need to distribute it as part of a vscode bundle. It would be really great if the mdn team would change the license to something like MIT or BSD. But I strongly urge you to clear this issue with legal before landing the patch.

@octref
Copy link
Contributor Author

octref commented Apr 18, 2018

Maybe we should omit "status": "standard" in order to make browser.js smaller.

@aeschli Sure, I can remove it together with a bunch of other unused properties.

@chriseppstein Yes, I submitted license review request for mdn/data. Before the review passes we won't ship the change. Meanwhile, can you chime in at mdn/data#199 for your concerns?

@octref
Copy link
Contributor Author

octref commented Apr 19, 2018

@chriseppstein

https://www.mozilla.org/en-US/MPL/2.0/FAQ/

Q11: How 'viral' is the MPL? If I use MPL-licensed code in my proprietary application, will I have to give all the source code away?
No. The license requires that Modifications (as defined in Section 1.10 of the license) must be licensed under the MPL and made available to anyone to whom you distribute the Source Code. However, new files containing no MPL-licensed code are not Modifications, and therefore do not need to be distributed under the terms of the MPL, even if you create a Larger Work (as defined in Section 1.7) by using, compiling, or distributing the non-MPL files together with MPL-licensed files. This allows, for example, programs using MPL-licensed code to be statically linked to and distributed as part of a larger proprietary piece of software, which would not generally be possible under the terms of stronger copyleft licenses.

We don't do any modification to the original mdn/data source, so vscode-css-languageservice could still be shared under MIT license.

@chriseppstein
Copy link

So, the legal definition of modification isn’t the same as a developer’s definition. I haven’t reviewed this PR in depth yet, but according to the lawyers I’ve worked with there’s a number of build time transformations that meet the legal definition of “modification” and would require a dual license.

If you don’t do anything like that, it’s still something that will require an ongoing attention towards.

@octref
Copy link
Contributor Author

octref commented Apr 20, 2018

@chriseppstein Thanks, I've opened mdn/data#210

@octref octref merged commit d83b784 into master Apr 23, 2018
@octref octref changed the title [WIP] Use MDN to enhance CSS LS. Part of #68 Use MDN to enhance CSS LS. Part of #68 Apr 23, 2018
@aeschli aeschli added this to the April 2018 milestone Apr 26, 2018
@octref octref deleted the octref/mdn branch May 23, 2018 21:10
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