feat: convert to ESM#56
Conversation
Convert package from CommonJS to native ES modules. All tests passing with no memory leaks.
kanongil
left a comment
kanongil
left a comment
There was a problem hiding this comment.
Moving to modules is probably a decent choice from an javascript ecosystem POV, but still a major pain for hapi. As such, I would rather keep it as is until the coverage and linting issue is better resolved.
| }; | ||
|
|
||
|
|
||
| export { header, Clip }; |
There was a problem hiding this comment.
I would much prefer if the exports are declared when defined, as it closer aligns with intentions of the style guide (to immediately know if a variable / function / class is exported).
| const { describe, it } = exports.lab = Lab.script(); | ||
| const lab = Lab.script(); | ||
| const { describe, it } = lab; | ||
| const expect = Code.expect; |
There was a problem hiding this comment.
I would prefer if expect is imported directly.
| "type": "module", | ||
| "repository": "git://github.com/hapijs/ammo", | ||
| "main": "lib/index.js", | ||
| "exports": "./lib/index.js", |
There was a problem hiding this comment.
We should probably also export package.json with a "./package.json": "./package.json" line.
| }, | ||
| "scripts": { | ||
| "test": "lab -a @hapi/code -t 100 -L -Y", | ||
| "test": "lab -a @hapi/code -Y", |
There was a problem hiding this comment.
Omitting coverage and linting is a big no-no IMO.
I know lab doesn't currently handle coverage checks for modules, so something needs to be done. Either we update the implementation, or find an acceptable alternative.
Same goes for linting, but it is probably more of a configuration issue.
There was a problem hiding this comment.
Yep yep, agreed. I'm omitting temporarily. This is still a TBD.
| "name": "@hapi/ammo", | ||
| "description": "HTTP Range processing utilities", | ||
| "version": "6.0.1", | ||
| "version": "7.0.0", |
There was a problem hiding this comment.
While this is clearly a breaking change, the version should not be updated in the PR.
| "scripts": { | ||
| "test": "lab -a @hapi/code -t 100 -L -Y", | ||
| "test": "lab -a @hapi/code -Y", | ||
| "test-cov-html": "lab -a @hapi/code -r html -o coverage.html" |
There was a problem hiding this comment.
npm run test-cov-html does not work.
2 participants
Summary
Test plan