Modernization + code cleanup ideas

[Daniel15]

I was just thinking about this while submitting a pull request for something else. This issue is mainly a braindump of ideas, and I'll likely split out ideas into separate issues to track them separately. I'm happy to work on some of these but I just wanted to kick off a discussion 😃

A relatively easy thing we could do to make the codebase a bit more modern is switch to newer ES6 syntax such as arrow functions. This should be pretty straightforward and some of it could be automated through jscodeshift transforms (eg. https://github.com/cpojer/js-codemod#arrow-function for arrow functions)

Using promises instead of callbacks would also be good. If you upgrade to Node 7.9 or higher, you could even totally skip promises and immediately go to async functions

Larger Things

Clean up code duplication in server.js

server.js has a loooot of copy and pasted code. For example:

• The string (svg|png|gif|jpg|json) appears 131 times.
• Every service needs to manually call cache
• Every service needs to manually call sendBadge(format, badgeData)

Some of this could be abstracted out into a reusable mechanism for registering services.

For example, imagine an API similar to this:

registerService({
  name: 'npm',
  type: 'Version', // Name of the group in try.html
  urlPattern: '/npm/v/{?user}/{repo}/{?tag}', // no extension, would be automatically handled.
  exampleData: { repo: 'react' },
  handler: (data, params) => new Promise((resolve, reject) => {
    callSomeAPIHere().then(version=> {
      resolve({
        text: version,
        color: versionColor(version),
      });
    });
  }),
})

or an async function:

  handler: async (data, params) => {
    const version = await callSomeAPIHere();
    return {
      text: version,
      color: versionColor(version),
    };
  }),

This would remove a lot of the boilerplate, abstract away the stuff the user shouldn't care about (like having to call cache), allow the try.html file to be auto-generated based on the provided metadata (name, type, and exampleData), and allow an API to programmatically get a list of all available badges (as per #776).

We wouldn't have to convert everything across all at once - The "new way" and "old way" could live side-by-side and we could move services across one at a time.

Split up server.js into several files

server.js is nearly 7000 lines long. That's a lot of code for a single file 😛 and would also make unit testing pretty difficult. We could start by pulling the utility methods (such as phpStableVersion, phpLatestVersion, latestVersion, versionColor, etc) out of server.js and into their own separate files. For example:

• utils/php-version.js: All the PHP version related stuff
• utils/response.js: sendSVG, sendOther, sendJSON and friends

That's all the easy ones. The bigger (and possibly more controversial change) is that I suggest splitting every individual service into its own separate file (eg. all the NuGet endpoints in a services/nuget.js file). We could automatically find all JS files in the services directory and register them on app startup. Special cases like the custom badges could remain in server.js.

As above, the services could be moved across one-by-one rather than doing everything at once.

What do you think? Am I totally crazy, or do these ideas sound reasonable?

[crazy-max]

Sounds great!

[paulmelnikow]

Love all these ideas!

Have been thinking about migrating to promises. It's an interesting point you make that async/await is on the horizon. To be honest I didn't realize it was coming so fast. Is it JavaScript's second golden age? We have 6k lines of service code, so refactoring it is a large undertaking, and skipping over promises is, well, promising. I love that sketch of the async/await handler function, and also your idea of calling these services rather than vendors.

The time for async/await doesn't seem quite here. Shields just upgraded from Node 5 to 6 (#921). Node 7 feels bleeding edge and 7.10 hasn't been released. Since @espadrine runs ops solo, so I defer to him about the risk of that sort of thing. Thaddée, I'm curious at what point you'd feel comfortable upgrading. This could be huge, if we could time it right.

Have had some offline discussions with @espadrine proposing similar things with server.js. I'll give him a chance to respond here.

Here are my thoughts: refactoring the services along the lines you suggest would make them more more easily read, modified, and tested, and more fun to work on. A collaborator on another project recently wrote, "no one likes writing code for old platforms," which I agree with, and also relates to outdated syntax and paradigms. I see value in auto-generating the content for try.html and in #776.

I have thoughts stewing about how we frame the service refactoring, which I need like a chance to collect, though here's the short version. I'd like to address the pain points of badge consumers conveyed in #777, #466, #701. I'm a product person too… so user focus is in my blood, what can I say? 😉

Meanwhile, if you're eager to start digging, why not start with the helper functions? When I discussed offline with @espadrine, he agreed they were worth extracting. To your list I'd add the cache function, the regularUpdate code, the analytics code, and the badge format helpers. IIRC #924 is the only open PR that touches any of that.

(Also worth reading #411, and perhaps pinging those folks.)

Glad to have you on board!

[Daniel15]

Node.js 8.x should be coming out this month (nodejs/node#10117), so 7.x shouldn't be considered "bleeding edge" for too much longer :)
http://node.green/ is a good reference for features you can use once you upgrade to a newer Node.js version.

also your idea of calling these services rather than vendors.

SPECIFICATION.md and CONTRIBUTING.md both use the word "service" rather than "vendor" so I wanted to keep that consistent 😄

Meanwhile, if you're eager to start digging, why not start with the helper functions?

Good idea :)

[paulmelnikow]

http://node.green/ is a good reference for features you can use once you upgrade to a newer Node.js version.

So useful. And, it looks like async functions are in 7.6 and 7.8! You can see in the tooltip that appears when hovering over 7.9.0.

[Daniel15]

Looks like they're even in 7.0.0 if you use the --harmony flag :)

[mgol]

Looks like they're even in 7.0.0 if you use the --harmony flag :)

They're behind a flag for a reason, though; they leak memory. I'd advise against using the --harmony flag. If you want to depend on async functions, either transpile or require Node >=7.9.

[Daniel15]

Started prototyping: #963

They're behind a flag for a reason, though; they leak memory.

Ahh, good to know! I guess we'd need to benchmark and see how they perform.

[paulmelnikow]

What do you think about adopting semistandard for new code? We could do this via eslint-config-semistandard which would give us the ability to add overrides and enforce additional rules if we need to.

I appreciate the premise of standard, which is that most style choices are "just pick something," and by having a "standard" you save a lot of time and energy having those debates in every code base. Some of the rules here and there I personally wouldn't choose. For example, I prefer to use a space after the ! operator for readability.

There's one that seems objectively worse, which is [comma-dangle: ['error', 'never']. standard is reluctant to make incremental improvements that break backward compatibility, so even though the arguments for ["error", "always-multiline"] are well articulated, there isn't a path for them to adopting it. See standard/standard#240. As reluctant as I am to open Pandora's box, comma-dangle is a rule I would prefer to override.

If we do go this route, we could start with service-tests/…

[paulmelnikow]

Adding @ahmadnassri @kyrias @jirutka who participated in #411 👋

[Daniel15]

My team at work have started using Prettier to format our code. It's very good. Prettier is more powerful than anything that's ESLint based, as all the fixes are based on pretty printing the AST (whereas ESLint's fixers are restricted to string manipulation so not everything can easily have a fixer). I'm really enjoying using it as I don't need to think about style nits any more.

We did used to have some ESLint style rules, but they required a lot of configuration. Prettier is opinionated (I guess in the same way as semistandard or standard) and works fine out-of-the-box with no extra config.

[webcaetano]

Split up server.js into several files

This !

We could split every service as folder and every bagde as new file
E.g.

github/forks.js
github/stars.js
jira/issue.js

[Daniel15]

@webcaetano That's an interesting idea! Currently I'm just playing with the idea of having one JS file per service (eg. services/github.js, services/jira.js, etc). You can see my prototype so far in #963.

[paulmelnikow]

Because I despise nitpicking stuff like indentation and spacing in pull request comments, I'd like to nudge forward our automated style checking, at least for new files being added.

I don't want to totally rewrite server.js just to get automated style checking… the blame tracking is just too useful. So let's it's just take care of that when we start splitting it out.

I'm open to pursuing Prettier at some point in the future. It seems a much better solution for formatting. However it doesn't handle the semantic issues, so it doesn't keep us from also needing an eslint config.

So, I suggest we add our preferred style rules to eslint config files and drop them in the lib/ and server-tests/ folders. I worked up a branch for comments. The goal of this pass was high value + moderate impact.

[paulmelnikow]

#1082 integrated Prettier and #1167 will turn it on.

Before we merge that, I want to argue for discontinuing the use of semicolons for lib/ and service-tests/ – that is, for new code – to reduce developer friction. I hate to open a can of worms, but the more I develop on projects which have updated their style, the clearer it is that I need to make this argument.

Since prettier is going to reindent a bunch of those files, now is the best time to do this.

It's been six months since I wrote this:

As a personal aside, I've stopped using semicolons. ASI is terrible. However, it can't be turned off. Placing the desired semicolons doesn't prevent ASI from inserting more bad ones. There are error cases of accidental insertion with both styles, which basically are a wash. You need a linter to avoid shooting yourself in the foot. Since semicolons are expensive – a whole character for every line of code! – and you need the linter anyway, they're a clear loss. I prefer to omit the semicolons and lint unexpected multiline statements and unreachable code, which prevent the errors.

Sources:

• https://www.youtube.com/watch?v=Qlr-FGbhKaI
• http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding

Since then, more and more of the projects I work on have modernized their tooling with linters.

Given a line beginning with a [, one of the rare cases requiring a semicolon at EOL or BOL, they will complain.

foo()
[1, 2, 3].forEach(...)

Eslint will detect an unexpected multiline statement and unexpected use of the comma operator.

As an experiment I turned on semi: false for prettier and re-ran it on #1167. There are semicolons inside of strings, inside comments, inside for expressions, and in a handful of places where there are two statements on a single line. There are literally zero instances where a semicolon is required at the end or beginning of a line. This is determined not by some manual process, but by a highly reliable one: eslint's automatic fixer, which has a carefully tested implementation of the rules of when a semicolon is needed.

Out of curiosity, I did the same for server.js. Same story. Of the 3,800 lines containing semicolons, only the 13 for expressions actually need them. Not a single necessary semicolon at beginning or end of line.

The semicolons in Shields create developer friction in two ways. One is that it takes time to place the semicolons in the right place and keep them in the right place during changes, especially in short / one-line functions, variable declarations, function expressions, and JSX. (As in #1163, which I just realized is not being linted. 😝) The other is that it introduces friction when I move between Shields and projects which don't use semis. When switching from Shields, I type semis into those projects for a while; when switching to Shields, the linter catches me for a while before I remember to switch over.

standard gains in popularity every day, and is the norm for new projects. It's clear that semicolon-less eventually will be the norm for the community, whether they adopt standard or not. So these frictions are only worsening as time goes by.

We will lose some blame tracking with this change, but I don't think it's a big deal. Whereas server.js has countless commits and 200+ contributors, most of the new files are short and have only a handful. It's not difficult to find out where a line in one of these files originated.

[Daniel15]

I want to argue for discontinuing the use of semicolons

oh no. haha. I'm fine either way as long as it's consistent, but I might need some convincing. 😛

Do any major JavaScript libraries use no semicolons? I'm likely biased since we're pro-semicolon at Facebook, but a number of large libraries using that style might persuade me that it's a good idea.

One is that it takes time to place the semicolons in the right place and keep them in the right place during changes

Prettier makes that easier. Just configure your editor to run Prettier on save. 😄

standard gains in popularity every day, and is the norm for new projects.

Is it? Which projects use it? I've seen many projects use Prettier which does use semicolons by default.

[paulmelnikow]

Do any major JavaScript libraries use no semicolons? I'm likely biased since we're pro-semicolon at Facebook, but a number of large libraries using that style might persuade me that it's a good idea.

These are some major projects which do not use semis:

• Bootstrap
• Vue
• Redux and react-redux
• Lodash
• npm
• Express (still lots of semis in the project, since code is brought up to standard at the time it's modified)
• request
• Karma
• Atom, Electron, and node-fetch
• Greenkeeper
• next.js
• Zepto
• fastify

Prettier makes that easier. Just configure your editor to run Prettier on save. 😄

I found recipes for pre-commit hooks, but ran into problems where the process would stage partially staged files. Maybe the editor approach is better! Regardless I should give that a try.

standard gains in popularity every day, and is the norm for new projects.

Is it? Which projects use it? I've seen many projects use Prettier which does use semicolons by default.

Yea, it's a fair question. My perception may be more anecdotal, based on scanning projects that are coming by in my feed, over time. I have seen that too. Prettier gained a lot of adopters fast. It's probably taking more of the new projects than standard. Though of course some of them are not using semis.

[paulmelnikow]

@Daniel15 My empirical experience, from time spent on this project vs. other projects, is that routine coding operations take less time without semis, making this a justifiable time saver. For people who use prettier inside their editors, it's transparent. I'd like to go ahead with this if you don't strongly object.

[Daniel15]

Sure :) I don't mind as long as it's all automated

[paulmelnikow]

We really should get Prettier turned on. (#1167) That work got started, so I think eslint is already ignoring everything formatting-related.

I still feel dropping semicolons is a good idea, and the impact is small relative to the benefit we'll get as we rewrite the services. Are there any objections? Honestly there are so many other things worth debating. If anyone really doesn't think we should do this, I'll let it drop.

[Daniel15]

Prettier is great! Do it! Internally at Facebook, 100 % of our JavaScript is now formatted using Prettier. :D

[Daniel15]

I think this can be closed, since we're on the right track now :)