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

add YAML to common languages #1952

Merged

Conversation

mojavelinux
Copy link
Contributor

resolves #1632

@mojavelinux
Copy link
Contributor Author

This change only increases the packed bundle size by < 1K (from 47330 bytes to 47988 bytes).

@marcoscaceres
Copy link
Contributor

This change only increases the packed bundle size by < 1K (from 47330 bytes to 47988 bytes).

Every byte counts :)

@marcoscaceres
Copy link
Contributor

I need to check what other languages are in the common category, and where YAML sits in relation to those languages in terms of usage. We can probably get those stats from Github, which would help us inform the decision (and maybe even drop some other languages if it turns out they have fallen out of fashion).

@mojavelinux
Copy link
Contributor Author

Well, I can tell you for sure that YAML is more widely used than apache, which is also in the common set. So if we are going by prominence, YAML is a clear winner.

@marcoscaceres
Copy link
Contributor

Well, I can tell you for sure that YAML is more widely used than apache, which is also in the common set. So if we are going by prominence, YAML is a clear winner.

Yes, absolutely. But looking for data/stats on which to make the decisions.

@mojavelinux
Copy link
Contributor Author

I also think it's rather odd to have JSON but not YAML (given that just about every CI server and devops automation tool uses YAML in some capacity, and not JSON). I'd also say that having http in the common set is rather esoteric. I've rarely encountered documentation that highlights http responses (and when it is needed, it's usually very domain specific, such as REST API docs).

@marcoscaceres
Copy link
Contributor

@mojavelinux I agree with what you are saying. I'm new to maintaining the project, so want to make sure that the decisions we make going forward are supported by data (specially anything that affects bundle size, even by less than a kb).

I can't comment as to why the current languages are in the "common" set, as I wasn't around when those decisions were made - but I'm sure the previous maintainers had good reasons so don't want to question their judgement.

All we can do is make an informed (i.e., data driven) decision - and then have a record of why we made a decisions to include something. We might not be able to get these stats, but we need some data points that can justify the decision (irrespective of what's already considered a "common" by the library).

Make sense?

@mojavelinux
Copy link
Contributor Author

mojavelinux commented Jan 25, 2019

I do understand where you're coming from. My point is that drawing the line on YAML is a strange hill to battle. The prominence of YAML hardly has to be debated. It's absence from highlight.js common sticks out like a sore thumb.

Let's get metrics when there's actually something to debate. This is like trying to get data to prove the sky is blue. We know that already.

@jf990
Copy link
Contributor

jf990 commented Jan 25, 2019

This is a bit of a losing battle. Today it’s a certain set of languages but next release that can/ probably will change. The problem arises some set of developers depend on the library obeying a contract , then next release those numbers change and then the contract changes. Now a language expected in the build disappeared. This is not a scalable solution. We really should migrate this to a no default language build and require building your own required language set. A few years ago yaml was not a concern but now it is ( I can use it) but what happens tomorrow after we migrate to the next set of most popular numbers? We take yaml out and the the yaml users are broken. And by yaml I mean any language take your pick.

@mojavelinux
Copy link
Contributor Author

mojavelinux commented Jan 25, 2019

Since I feel like I have a strong case, I decided to run a Twitter poll anyway. I'll post the results once they are up. That should at least prove with data that I'm not representing one voice.

UPDATE: So far 61 votes, 89% in favor of adding YAML. I'm going to let the poll run for several more days to get a broader sample set.

UPDATE: Final vote tally. 152 votes, still 89% in favor of adding YAML. See https://twitter.com/mojavelinux/status/1088642433703792641

@marcoscaceres
Copy link
Contributor

A few years ago yaml was not a concern but now it is ( I can use it) but what happens tomorrow after we migrate to the next set of most popular numbers? We take yaml out and the the yaml users are broken. And by yaml I mean any language take your pick.

yeah, that's also a very valid point. We are taking this approach more generally with hljs: we are moving all new languages out of the core repo and asking people to bundle them separately.

@mojavelinux
Copy link
Contributor Author

Keep in mind, that just makes using highlight.js from a CDN more tedious because now you have N + 1 requests where N is the number of languages. That's hardly better than a minuscule increase in bundle size.

Another possible path to take is a language bundle that's prepared for cdnjs specifically. Because that's really the main use case for the common build anyway. What we're really debating is what highlight.min.js delivers. Outside of that, people are making their own bundles away (which is a fine approach, but requires extra effort that turns people off of using highlight.js).

Interesting to note that Prism also has a core language bundle, but it's even more narrowly scoped than the common set in highlight.js. So you end up back at that N + 1 request problem when using it from a CDN like cdnjs.

Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Ok, let’s do this.

@marcoscaceres marcoscaceres merged commit 904e8fc into highlightjs:master Aug 17, 2019
@mojavelinux
Copy link
Contributor Author

mojavelinux commented Aug 17, 2019 via email

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.

Include yaml in common language set
3 participants