-
Notifications
You must be signed in to change notification settings - Fork 160
Conversation
👍 This keeps the repo a lot lighter / cleaner I like pulling in the tests |
I'll fix those linting issues. |
@jdalton look like you need to sign the CLA too |
Ok, signed the CLA. I'm not sure why the other test fails are occurring, they pass when doing |
url = require('url'); | ||
|
||
var testPath = path.resolve(path.join(__dirname, '..', '..', pkg.path)), | ||
token = 'Mjk5ZGQxNTk0ZDA3YTllY2I5YzlmMzRhZWYyOTEyZTQ1MDE2ZDdmNw==', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of relying on someone's personal token for this task to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a no-scope public token (so harmless, readonly access). It allows getting the higher 50k per hour request limit.
I still think this change isn't necessary, but I'll defer to the rest of the team. |
What's nice about this approach is:
|
"intern": "2.2.2" | ||
"intern": "2.2.2", | ||
"lodash": "^3.9.3", | ||
"request": "^2.58.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it looks like you have to download a half of the internet
That's a bit of a stretch. It dependencies aren't egregious and it only takes ~5 seconds to download.
Why not to use something more
Because I've used request
and I trust it, it's the 4th (going on 3rd) most depended on package in npm, and it's a dev dep to boot (so not installed very frequently and only when deving).
This PR automates the retrieval of w3c tests by an entry in the package.json