Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#3751: Esm interop flag #3616

Merged
merged 1 commit into from Jan 14, 2020
Merged

#3751: Esm interop flag #3616

merged 1 commit into from Jan 14, 2020

Conversation

D10221
Copy link
Contributor

@D10221 D10221 commented Jan 7, 2020

Enable esm interop behind a flag
Enable experimental, optional use
Minimum changes
Tests: migration/seeds/configuration
fix: eslint error

Enable esm interop behind a flag
Enable experimental, optional use
Minimum changes
Tests: migration/seeds/configuration
fix:  eslint error
return assertExec(
`node ${KNEX} --esm migrate:latest --knexfile=test/jake-util/knexfile-esm/knexfile.js --knexpath=../knex.js`
).then(({ stdout }) => {
assert.include(stdout, 'Batch 1 run: 1 migrations');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cli-testlab 1.10.0 supports multiple assertions, so if you bump dependency, you can do

return assertExec(
    `node ${KNEX} --esm migrate:latest --knexfile=test/jake-util/knexfile-esm/knexfile.js --knexpath=../knex.js`, {
      expectedOutput: ['Batch 1 run: 1 migrations', 'one.js']
    }
  )

`node ${KNEX} --esm seed:run --knexfile=test/jake-util/knexfile-esm/knexfile.js`
).then(({ stdout }) => {
assert.include(stdout, 'Ran 1 seed files');
assert.notInclude(stdout, 'first.js');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use notExpectedOutput and expectedOutput params for assertExec to avoid then chaining

@kibertoad
Copy link
Collaborator

Thanks! There are tiny little things left, and then this can be merged :)

@kibertoad
Copy link
Collaborator

Could you also please submit a PR with documentation for this on https://github.com/knex/documentation?

@D10221
Copy link
Contributor Author

D10221 commented Jan 14, 2020

Thanks,
Would you like me to implement the remarks, verbatim?
Or its an opportunity ? :)
Should I add it to 'test/cli/knexfile-test.spec.js' ?
Sorry, I thought the case was covered by the Jake (integration?)test
I'll check the docs

@kibertoad
Copy link
Collaborator

@D10221 oh, you are right, this particular file is not using cli-testlab yet, so please ignore that comment, I'll refactor myself later.

@D10221
Copy link
Contributor Author

D10221 commented Jan 14, 2020

Thanks.
Opened a doc's PR
Let me know if its aligned to 'expectations' :)

@kibertoad kibertoad merged commit 6508602 into knex:master Jan 14, 2020
@kibertoad
Copy link
Collaborator

Released in 0.20.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants