Improve Support for Liftoff's Preloaders #3613

briandamaged · 2020-01-05T22:22:03Z

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

... Specifically: - resolveDefaultKnexfilePath(...) - resolveKnexFilePath(...)

This property is used when inferring the appropriate extension for the migration files

briandamaged · 2020-01-05T22:26:39Z

bin/cli.js

-  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('.', '');
-    }
-  }
-


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

briandamaged · 2020-01-05T22:28:55Z

bin/cli.js

-    knexfile: argv.knexfile,
-    knexpath: argv.knexpath,
+    configPath: argv.knexfile,


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.

briandamaged · 2020-01-05T22:31:25Z

bin/utils/cli-config-utils.js

-    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`


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.

briandamaged · 2020-01-05T22:32:03Z

bin/utils/cli-config-utils.js

-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}`;
-}
-


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

briandamaged · 2020-01-05T22:41:49Z

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 · 2020-01-05T22:54:46Z

@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 · 2020-01-05T23:09:22Z

@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.

... 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 · 2020-01-06T04:01:41Z

@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:

gulpjs/liftoff#55

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

briandamaged · 2020-01-06T17:29:52Z

@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 · 2020-01-06T22:16:01Z

@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 · 2020-01-28T01:38:15Z

@briandamaged Would you like any help with this?

briandamaged · 2020-01-28T01:50:36Z

@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 · 2020-01-29T19:02:46Z

@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:

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.
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 · 2020-01-29T19:32:09Z

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 · 2020-01-29T22:25:34Z

@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.

briandamaged · 2020-01-29T22:42:39Z

package.json

+    "branches": 69,
+    "extension": [
+      ".js"
+    ]


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 · 2020-01-30T00:29:31Z

Thanks! I'll take a look tomorrow.

briandamaged · 2020-01-30T22:40:00Z

@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 · 2020-01-31T00:12:00Z

@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 · 2020-01-31T00:37:00Z

@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 · 2020-01-31T00:39:12Z

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 · 2020-01-31T17:15:48Z

@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)

briandamaged · 2020-01-31T17:16:30Z

bin/cli.js

+//      ensures that Liftoff will then resolve the path to `--knexfile` correctly.
+if (argv.cwd) {
+  process.chdir(argv.cwd);
+}


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

briandamaged · 2020-02-08T16:17:39Z

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

kibertoad · 2020-02-08T16:20:36Z

@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 · 2020-02-08T16:21:41Z

@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?

kibertoad · 2020-02-08T16:25:29Z

@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 · 2020-02-08T16:58:04Z

@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 · 2020-02-08T17:00:27Z

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

Brian Lauber added 4 commits January 4, 2020 19:22

CLI uses Liftoff to handle knexfile discovery

6fabb35

Dead code removal: config resolver functions...

516d2e5

... Specifically: - resolveDefaultKnexfilePath(...) - resolveKnexFilePath(...)

Backwards compatibility w/ the undocumented config.ext property

e820a2b

This property is used when inferring the appropriate extension for the migration files

Removed invalid knexfile/knexpath args from Liftoff#launch(..)

772c47d

briandamaged commented Jan 5, 2020

View reviewed changes

Brian Lauber added 4 commits January 5, 2020 20:48

Added some specs for --cwd and --knexfile interaction

10bb520

Fix: removed the ".only" qualifier from the testcase

9ea3f5c

Added some migrations to the knexfile-relative tests...

689e3eb

... this way, we can be sure that the correct migrations are being run. (As opposed to a directory lookup error, or something like that)

Added test case to verify that CWD is changed correctly

74984a5

Merge branch 'master' into fix/knexfile-cli-arg

5a79595

briandamaged commented Jan 29, 2020

View reviewed changes

Added logic to replicate the "classic" handling for --knexfile

2a5aceb

briandamaged commented Jan 31, 2020

View reviewed changes

kibertoad approved these changes Feb 8, 2020

View reviewed changes

briandamaged changed the title ~~WIP: Improve Support for Liftoff's Preloaders~~ Improve Support for Liftoff's Preloaders Feb 8, 2020

kibertoad merged commit 947273e into knex:master Feb 8, 2020

petar-lazarov mentioned this pull request Apr 19, 2024

Do not overwrite cwd if knexfile and cwd is passed #6060

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Support for Liftoff's Preloaders #3613

Improve Support for Liftoff's Preloaders #3613

briandamaged commented Jan 5, 2020

briandamaged Jan 5, 2020

briandamaged Jan 5, 2020

briandamaged Jan 5, 2020

briandamaged Jan 5, 2020

briandamaged commented Jan 5, 2020

kibertoad commented Jan 5, 2020

briandamaged commented Jan 5, 2020

briandamaged commented Jan 6, 2020

briandamaged commented Jan 6, 2020

kibertoad commented Jan 6, 2020

kibertoad commented Jan 28, 2020

briandamaged commented Jan 28, 2020

briandamaged commented Jan 29, 2020

briandamaged commented Jan 29, 2020

briandamaged commented Jan 29, 2020

briandamaged Jan 29, 2020

kibertoad commented Jan 30, 2020

briandamaged commented Jan 30, 2020

briandamaged commented Jan 31, 2020

kibertoad commented Jan 31, 2020

kibertoad commented Jan 31, 2020

briandamaged commented Jan 31, 2020

briandamaged Jan 31, 2020

briandamaged commented Feb 8, 2020

kibertoad commented Feb 8, 2020

briandamaged commented Feb 8, 2020

kibertoad commented Feb 8, 2020

briandamaged commented Feb 8, 2020

kibertoad commented Feb 8, 2020

Improve Support for Liftoff's Preloaders #3613

Improve Support for Liftoff's Preloaders #3613

Conversation

briandamaged commented Jan 5, 2020

briandamaged Jan 5, 2020

Choose a reason for hiding this comment

briandamaged Jan 5, 2020

Choose a reason for hiding this comment

briandamaged Jan 5, 2020

Choose a reason for hiding this comment

briandamaged Jan 5, 2020

Choose a reason for hiding this comment

briandamaged commented Jan 5, 2020

kibertoad commented Jan 5, 2020

briandamaged commented Jan 5, 2020

briandamaged commented Jan 6, 2020

briandamaged commented Jan 6, 2020

kibertoad commented Jan 6, 2020

kibertoad commented Jan 28, 2020

briandamaged commented Jan 28, 2020

briandamaged commented Jan 29, 2020

briandamaged commented Jan 29, 2020

briandamaged commented Jan 29, 2020

briandamaged Jan 29, 2020

Choose a reason for hiding this comment

kibertoad commented Jan 30, 2020

briandamaged commented Jan 30, 2020

briandamaged commented Jan 31, 2020

kibertoad commented Jan 31, 2020

kibertoad commented Jan 31, 2020

briandamaged commented Jan 31, 2020

briandamaged Jan 31, 2020

Choose a reason for hiding this comment

briandamaged commented Feb 8, 2020

kibertoad commented Feb 8, 2020

briandamaged commented Feb 8, 2020

kibertoad commented Feb 8, 2020

briandamaged commented Feb 8, 2020

kibertoad commented Feb 8, 2020