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

Improve Support for Liftoff's Preloaders #3613

Merged
merged 11 commits into from Feb 8, 2020

Conversation

@briandamaged
Copy link
Collaborator

briandamaged commented Jan 5, 2020

Revises the Knex CLI's initialization logic so that it takes better advantage of Liftoff's module pre-loading. More specifically:

  • Removed the unsupported "knexfile" and "knexpath" arguments from the call to Liftoff#launch(..).
  • Removed Knex's custom logic for config file discovery. (In certain cases, this logic would fail to discover configuration files unless they were either Javascript or Typescript. Furthermore, this logic would never invoke Liftoff's module pre-loaders, which could then lead to syntax errors when opening the files)

Importantly: this PR changes the CLI's behavior when both the --cwd and --knexfile parameters are specified. Previously, the location of the --knexfile was always relative to the value specified via --cwd. Once this PR is merged, the two parameters will be independent of one another. This better aligns w/ the semantics used by Liftoff. For example:

knex --knexfile /path/to/shared/knexfile.js --cwd /var/www/project1 migrate:latest
knex --knexfile /path/to/shared/knexfile.js --cwd /var/www/project2 migrate:latest
briandamaged added 4 commits Jan 5, 2020
... Specifically:
 - resolveDefaultKnexfilePath(...)
 - resolveKnexFilePath(...)
This property is used when inferring the appropriate extension
for the migration files
if (!opts.knexfile) {
const configurationPath = resolveKnexFilePath();
const configuration = configurationPath
? require(configurationPath.path)
: undefined;

env.configuration = configuration || mkConfigObj(opts);
if (!env.configuration.ext && configurationPath) {
env.configuration.ext = configurationPath.extension;
}
}
// If knexfile is specified
else {
const resolvedKnexfilePath = path.resolve(opts.knexfile);
const knexfileDir = path.dirname(resolvedKnexfilePath);
process.chdir(knexfileDir);
env.configuration = require(resolvedKnexfilePath);

if (!env.configuration) {
exit(
'Knexfile not found. Specify a path with --knexfile or pass --client and --connection params in commandline'
);
}

if (!env.configuration.ext) {
env.configuration.ext = path
.extname(resolvedKnexfilePath)
.replace('.', '');
}
}

Comment on lines -42 to -76

This comment has been minimized.

Copy link
@briandamaged

briandamaged Jan 5, 2020

Author Collaborator

This logic was essentially duplicating the "configuration file discovery" logic that was already provided by Liftoff. However, modules discovered via this control path would never utilize any of Liftoff's module pre-loaders. This is the reason that some users were observing syntax errors when attempting to load non-JS config files via the --knexfile parameter. Ex: #2998

knexfile: argv.knexfile,
knexpath: argv.knexpath,
configPath: argv.knexfile,
Comment on lines -370 to +354

This comment has been minimized.

Copy link
@briandamaged

briandamaged Jan 5, 2020

Author Collaborator

The Liftoff#launch(..) method does not understand the knexfile and knexpath parameters. Furthermore, it does not pass them though to the env object when calling the invoke(..) callback.

const path = resolveDefaultKnexfilePath();
throw new Error(
`No default configuration file '${path}' found and no commandline connection parameters passed`
`No configuration file found and no commandline connection parameters passed`
Comment on lines -11 to +12

This comment has been minimized.

Copy link
@briandamaged

briandamaged Jan 5, 2020

Author Collaborator

This error message was rendering the config file name as knexfile.undefined. This is because the resolveDefaultKnexfilePath() was expecting an argument for the desired extension.

However, Knex actually supports many different config file extensions, including .ts and .coffee. So, it didn't really make sense to just show the .js extension. Therefore, I removed the name from the error message.

function resolveKnexFilePath() {
const jsPath = resolveDefaultKnexfilePath('js');
if (fs.existsSync(jsPath)) {
return {
path: jsPath,
extension: 'js',
};
}

const tsPath = resolveDefaultKnexfilePath('ts');
if (fs.existsSync(tsPath)) {
return {
path: tsPath,
extension: 'ts',
};
}

console.warn(
`Failed to find configuration at default location of ${resolveDefaultKnexfilePath(
'js'
)}`
);
}

function resolveDefaultKnexfilePath(extension) {
return process.cwd() + `/knexfile.${extension}`;
}

Comment on lines -34 to -61

This comment has been minimized.

Copy link
@briandamaged

briandamaged Jan 5, 2020

Author Collaborator

Dead code. (This detail is handled by Liftoff now)

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 5, 2020

FYI: This code currently breaks 1 unit test. Specifically:

  6) CLI tests
       resolves knexfile correctly with cwd specified:
     node /home/brian/src/arclia/knex/bin/cli.js migrate:latest --cwd=test/jake-util/knexfile --knexfile=knexfile.js -> FAIL. 
Stdout:  
Error: /home/brian/src/arclia/knex/bin/utils/cli-config-utils.js:11
    throw new Error(
    ^

This error occurs because Liftoff treats --knexfile and --cwd as orthogonal parameters. More specifically, --knexfile is not evaluated relative to --cwd.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 5, 2020

@briandamaged I am somewhat concerned about breaking scripts for existing users, but I guess just one case with cwd is not too bad, as long as we publish it as a major version. Can you add one more cli-testlab-based test that was failing prior to this change, so that we don't break your fix again in the future? I guess adding ts file would work.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 5, 2020

@kibertoad : Sure! I just wanted to get your feedback on the breaking change before moving forward w/ the other test cases. I should have some time to write them in the next day or so.

briandamaged added 4 commits Jan 6, 2020
... this way, we can be sure that the correct migrations are
being run.  (As opposed to a directory lookup error, or something
like that)
@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 6, 2020

@kibertoad : I've added some new tests, but I'm noticing a discrepancy.

When both the --cwd and --knexfile flags are specified, it is changing the process's CWD to path.dirname(knexfile) (instead of honoring the --cwd value that was passed in explicitly). It looks like this is potentially a bug in the Liftoff library itself:

js-cli/js-liftoff#55

Anyway, I'll do a bit more digging into this tomorrow.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 6, 2020

@kibertoad : A closely-related question: what is the intention of the cwd value? Is it supposed to represent:

  • The directory that contains the Knexfile?
  • The directory that contains the developer's application? (ie: project root)
  • An arbitrary directory based upon the specific project's needs?
  • Something else?
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 6, 2020

@briandamaged Prior discussions on this subject: #2959 and #2952
I would say that CWD is arbitrary point that is the root from which other paths are calculated. Important behaviour to preserve as this is how knex used to work historically - paths to seeds and migrations are resolved relatively to knexfile. Path to knexfile, however, should be resolved relatively to CWD, whatever user decides to pass as such.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 28, 2020

@briandamaged Would you like any help with this?

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 28, 2020

@kibertoad : Howdy, and thx for following up! I got pulled into something else during the last few weeks, so I haven't been able to investigate this any further. However, I should have some time in the next few days.

Once I get back to this, I'll create a table that compares the "Old Behavior" and "New Behavior" for various scenarios. From there, we can then discuss what the desired behavior in each scenario should be, and implement things accordingly. (Saying it another way: I think the "New Behavior" fixes certain problems, but it might introduce new problems as well. So, we need to make sure we agree upon the requirements / expectations).

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 29, 2020

@kibertoad : Alright, here's the table demonstrating the behaviors for various command line arguments:

https://docs.google.com/spreadsheets/d/19TMouhxdIosYGzubO-2lbxm8Ykx4jR-Iu74HXohFYMg/edit?usp=sharing

As you can see:

  1. The master branch is effectively ignoring the --knexfile input. This is the reason that ts-node was not being registered properly when loading Typescript migrations.
  2. In the fix/knexfile-cli-arg branch, if --cwd is omitted, then the value of env.cwd will be inferred from --knexfile. So, this aligns nicely w/ the "All paths within the Knexfile are relative to the Knexfile's directory" requirement that was mentioned... somewhere? (I can't seem to find the documentation on it now)

So, based upon my understanding of the requirements, it looks like the behavior in fix/knexfile-cli-arg is correct.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 29, 2020

As for the travis-ci failure: it looks like the ts-node dev dependency probably was not installed for that particular scenario. Is there an easy way to confirm that?

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 29, 2020

@kibertoad : Okay, new update around the travis-ci error: it appears that nyc uses the following default:

  --extension, -e             a list of extensions that nyc should handle in
                              addition to .js
                   [string] [default: [".js",".cjs",".mjs",".ts",".tsx",".jsx"]]

This causes the value of require.extensions to be modified once nyc hands control off to mocha.

The problem: notice that ".ts" is already on that list. This causes Liftoff to believe that Typescript support has already been registered properly. Consequently, it decides that it does not need to register ts-node, which then causes 🔥 when it attempts to open the Knexfile.

The "quick fix" would be to restrict nyc to a specific set of extensions; however, I suspect that will introduce more problems in the future. (Especially if portions of Knex ever get rewritten in Typescript). I'm also starting to think that Liftoff might be introducing more problems than it actually solves.

... Specifically: the default configuration for nyc causes it
to add ".ts" to the the `require.extensions` list.

The problem: Liftoff consults `require.extensions` when deciding
which modules need to be preloaded.  Since ".ts" is already on
the list, Liftoff decides that "ts-node" must have already been
preloaded.  Therefore, it hands control off to the Knex CLI
without ever registering Typescript support.  Consequently, the
Knex CLI will explode if it attempts to open a Knexfile that was
written in Typescript.

In other words: this error only occurs when `nyc` (or another
tool) modifies `require.extensions` before handing control over
to the Knex CLI.
"branches": 69,
"extension": [
".js"
]

This comment has been minimized.

Copy link
@briandamaged

briandamaged Jan 29, 2020

Author Collaborator

FYI: This is the nyc fix that I mentioned here:

#3613 (comment)

Short story: it appears that nyc's extension handling interferes with Liftoff's extension handling. Thus, when you run the unit tests under nyc, it fails to preload ts-node properly.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 30, 2020

Thanks! I'll take a look tomorrow.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 30, 2020

@kibertoad : FYI: I just added a couple more rows to the spreadsheet. These ones are for:

  • An absolute --cwd path
  • A relative --knexfile path

As you can see, the two branches behave a bit differently for this specific scenario. So, which of these behaviors do we want to keep?

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 31, 2020

@kibertoad : Forgot to mention: I think I figured out an adjustment to the new code that will allow it to recreate the old behavior. To recap, let's say that:

  • Shell's CWD: /private/tmp/foo
  • --cwd : /private/tmp/bar
  • --knexfile : ./knexfile.js

In this case, the CLI will ultimately resolve the Knexfile at:

  • Old Behavior: /private/tmp/bar/knexfile.js
  • New Behavior: /private/tmp/foo/knexfile.js

Personally, I think that the New Behavior makes more sense. That is: paths are always relative to the Shell's CWD.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 31, 2020

@briandamaged I agree that this makes more sense; however, this would be a breaking change and would require bumping second number. Could you also update https://github.com/knex/knex/blob/master/UPGRADING.md explaining under which circumstances users are likely to experience change in behaviour, and what they would need to change to get same result as before?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 31, 2020

If we can keep the way to recreate the old behaviour, though, that would be very helpful, as in general knex tries to be as conservative as possible with breaking changes.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Jan 31, 2020

@kibertoad : Alright -- I went ahead and implemented a work-around that recreates the "classic" behavior. So:

  • If --cwd is specified, then --knexfile will be resolved relative to --cwd. Otherwise, --knexfile will be resolved relative to the shell's CWD.
  • Regardless of how --knexfile gets resolved, it will always invoke the appropriate Liftoff preloaders. (ex: it will recognize that the file is Typescript and register the ts-node module)
// ensures that Liftoff will then resolve the path to `--knexfile` correctly.
if (argv.cwd) {
process.chdir(argv.cwd);
}

This comment has been minimized.

Copy link
@briandamaged

briandamaged Jan 31, 2020

Author Collaborator

@kibertoad : Here's the work-around that allows the CLI to retain the "classic" behavior.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 8, 2020

@kibertoad : Howdy again! Just following up to see if there's anything you need on my side. Thx!

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 8, 2020

@briandamaged Sorry, just started at new job and got distracted. This looks great and I would be happy to merge this. Any reasons why you kept WIP in title?

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 8, 2020

@kibertoad : Ah -- nope. I just forgot to remove the WIP: prefix. Lemme take care of that now...

Good luck on your new job, btw! Doing something interesting, I hope?

@briandamaged briandamaged changed the title WIP: Improve Support for Liftoff's Preloaders Improve Support for Liftoff's Preloaders Feb 8, 2020
@kibertoad kibertoad merged commit 947273e into knex:master Feb 8, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 8, 2020

@briandamaged Thanks! Oh yeah, brand new team, greenfield project, I'm extremely hyped about it. The only unfortunate thing is that I switched stacks and returned to Java world, so there'll be less eating of own's dog food, unfortunately.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 8, 2020

@kibertoad: Ah -- oh well. I've heard that the Java language / ecosystem has improved significantly over the last several years. So, it might not be that painful. Plus, there are several languages that compile into Java bytecode, such as Kotlin and Scala. So, you might be able to integrate those into whatever you're building.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 8, 2020

Well yeah, convincing folks over there to use Kotlin is one of my current dreams :D

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.