feat: add support for node 12 #5
Conversation
… for node 12 support
| c: [, , '3'], | ||
| d: [, , '4', '5'], | ||
| e: [, , '6', , 'hello'], | ||
| g: [, ,] // eslint-disable-line comma-spacing |
There was a problem hiding this comment.
Latest standard complains about spaces after , and also having a space before ].
|
Build broken on node 6 - I'm going to remove support for node 6 (let me know if you disagree with this decision and we can discuss what to do here) :) |
|
having no context, i just might request that coverage go to 100% here to have an added assurance |
|
If getting the test coverage up to 100% is prohibitive, I think at the very least what we should do is audit the lines that the coverage report say are missing and make sure we don't have another instance like with mkdirp where the function signature changed. |
src/writer.js
Outdated
| } | ||
| }) | ||
| mkdirp(dir).then( | ||
| () => { resolve() }, |
There was a problem hiding this comment.
Can you just pass the resolve function itself here? 🤔
There was a problem hiding this comment.
@lumaxis - I'm not sure - can you elaborate? (Sorry - my foo.js is weak 😂 )
There was a problem hiding this comment.
mkdirp(dir).then(resolve, (err) => {})There was a problem hiding this comment.
I.e. instead of calling resolve() you can just pass a reference to resolve function.
There was a problem hiding this comment.
Cool - thanks! I'll try this (after I've got the tests working at all locally again :/ )
There was a problem hiding this comment.
@owenniblock interrupting just for the js syntax -- is there a test that will ensure that we get into that error block? the syntax in this file doesn't look like js promises, but i don't want to suggest a change from that perspective, just from "is there a test that ensures we get to this block, and is it passing" and let the code be what it needs to.
There was a problem hiding this comment.
I believe I have all the lines covered by tests now so hopefully we've covered all eventualities.
| @@ -114,4 +116,63 @@ describe('Config Mapper', function () { | |||
| writer.hasFiles(hashFive).should.equal(false) | |||
| writer.hasFiles(hashSix).should.equal(true) | |||
| }) | |||
|
|
|||
| it('should handle missing packagePath by not adding start', function () { | |||
| sinon.stub(fs, 'existsSync').returns(false) | |||
There was a problem hiding this comment.
Do we have to stub this if the file really doesn't exist?
There was a problem hiding this comment.
From memory - this was the simplest way to trigger the code - logic is as follows:
- Find the current project's package.json
- Check that the path exists
- If it does - perform the logic to get the
startelement - If it doesn't - don't add
startelement
Because the logic picks up the package.json from the kickerd project it was much simpler to stub the call to get this file so we could pretend it doesn't exist.
spec/config-mapper.spec.js
Outdated
| const stubPathResolve = sinon.stub(path, 'resolve') | ||
| stubPathResolve.onCall(0).returns(replacementPackagePath).callThrough() | ||
|
|
||
| const noPackagePath = configMapper.load('./spec/.kickerd.toml.one') |
There was a problem hiding this comment.
I'm a little confused as to why we pass in a fake name here and then try to stub the calls that the code uses to return the real file. Why not pass the actual filename here instead?
There was a problem hiding this comment.
We're bypassing the call which would normally pick up the package.json from the root of kickerd - we point it to a test .json file which includes a start element in scripts and then check that we're including the custom start element.
| @@ -33,10 +33,16 @@ function initConfig (hash) { | |||
| return hash | |||
| } | |||
|
|
|||
| // NOTE: These tests cannot be run in isolation | |||
There was a problem hiding this comment.
😢
Out of curiosity, what's causing this limitation?
There was a problem hiding this comment.
These tests call out to an external etcd service which keeps state. We should probably look at mocking out these requests and responses at some point but it felt like I'd already spent long enough adding the tests to get the coverage up so I think that's a task for another day!
Syntax highlighting in CONTRIBUTING.md Co-authored-by: Lukas Spieß <lumaxis@github.com>
Co-authored-by: Lukas Spieß <lumaxis@github.com>
Co-authored-by: Lukas Spieß <lumaxis@github.com>
Co-authored-by: Lukas Spieß <lumaxis@github.com>
Co-authored-by: Lukas Spieß <lumaxis@github.com>
…d into fix/update-for-node12
owenniblock
changed the title
owenniblock
changed the title
Motivation
We need to build things which are written with node 12. This breaking change will add node 12 support to this module.
Notes
I've updated all dependencies to the very latest versions (explicitly where not already handled by the
package.jsonversioning). Tests are green but it you feel test coverage isn't sufficient to feel comfortable releasing this - let me know and we can discuss adding further tests.