search

mdn / short-descriptions

Short descriptions of web platform features, for flexible usage in applications.
Other
12 stars 6 forks source link

Make it possible to require short-descriptions #17

Closed ddbeck closed 5 years ago

ddbeck commented 5 years ago

This PR adds an index.js file so you can properly require this package. It loads up the JSON files, compiles them into a single object, and exports that object. In other words, I'm totally copying after browser-compat-data.

~This PR will be a draft until #15 is merged, as my branch is based on that PR.~

Also, this PR is a major piece of #13.

ddbeck commented 5 years ago

@wbamberg hey Will, this is one of my tasks for this sprint. If you do not expect to have time to review this this week or next, let me know—just so I can update the spreadsheet one way or the other. Thank you!

wbamberg commented 5 years ago

@ddbeck , yeah I know, sorry Daniel. I will get to it imminently, I promise!

wbamberg commented 5 years ago

I wasn't really sure how to test this, so I tried just making my own tiny test node script, checking your your branch under "node_modules", then calling require('short-descriptions') from my test script.

With that I get an error like: "Error: ENOENT: no such file or directory, scandir './descriptions'". I tried adding a line like:

  const dir = path.resolve(__dirname, './descriptions');

at the start of loadJSON() and then it seemed to work OK. I don't know if this is a legitimate bug in your script or is just my hokey test.

The code looks fine and is very clear. You have a blank line at line 3 which doesn't seem necessary.

ddbeck commented 5 years ago

Thank you, Will! That's a good catch and the right solution for the file path situation. I'm traveling and fighting lousy wifi, but as soon as I can I'll push a fix. And maybe brainstorm a way of validating the package install. Thank you again!

ddbeck commented 5 years ago

OK, I've added the two fixes you suggested (and sorted the requires).

I also figured out an OK pattern for testing the built package: npm pack, copy the resulting archive into a new, empty package, and run npm install mdn-short-descriptions-0.x.y.tgz. Since that sits atop the usual npm test machiery, it seems like something to add as a shell script for the Travis CI config, when I get to it, rather than including in this PR. Does that sound OK?