brunolm commented 5 years ago

Environment

Knex version: 0.16.3 Database + version: docker postgres:10.4 OS: Windows 10 Pro & Docker node:8.14.0

Bug

knex migrate:make --knexfile knexfile.ts
Error message: Unexpected token import

Works normally on 0.15.x

mikl commented 5 years ago

0.16 now has TypeScript types included in the repo (see #2845 ). You probably need to remove the @types/knex from your project for this to work correctly.

brunolm commented 5 years ago

I uninstalled @types/knex but I still got the error.

/usr/src/app/src/db/knexfile.ts:1
(function (exports, require, module, __filename, __dirname) { import { database } from '../config'
                                                              ^^^^^^

SyntaxError: Unexpected token import
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:617:28)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at initKnex (/usr/src/app/node_modules/knex/bin/cli.js:62:25)
    at Command.commander.command.description.option.action (/usr/src/app/node_modules/knex/bin/cli.js:172:24)
    at Command.listener (/usr/src/app/node_modules/commander/index.js:315:8)
    at emitTwo (events.js:126:13)
    at Command.emit (events.js:214:7)
    at Command.parseArgs (/usr/src/app/node_modules/commander/index.js:654:12)
    at Command.parse (/usr/src/app/node_modules/commander/index.js:474:21)

elhigu commented 5 years ago

knex migrate:make --knexfile knexfile.ts isn't that trying to run typescript code with plain node? I have no idea how that could have worked with 0.15 either.

How does your knexfile look like? is there any import statements?

brunolm commented 5 years ago

import * as knex from 'knex'
import * as path from 'path'
import { env } from './env'

const database = {
  client: 'postgresql',
  connection: env.databaseUrl,
  migrations: {
    directory: path.resolve('../db/migrations'),
    tableName: 'knex_migrations',
  },
  seeds: {
    directory:  path.resolve('../db/seeds'),
  },
} as knex.Config

export = database

works 100% on 0.15

mikl commented 5 years ago

I have no idea why that would work with 0.15. As the error Unexpected token import tells you, the import * as knex from 'knex' is not understood by Node.js (not in the configuration you're using, at least).

You'll either need to compile your knexfile to JavaScript, use a Node.js version that understands the import syntax (see https://nodejs.org/api/esm.html ), or revert the file to use the old require() syntax.

elhigu commented 5 years ago

I suppose there was a bug in knex 0.15 so that it never read that knexfile. Doesnt look like bug in 0.16 that shouldnt work.

brunolm commented 5 years ago

so that it never read that knexfile

It does a 100% read the knexfile, I use it in more than a single project (from the top of my head I can think of 4, in production) with different migration folders and connection data.

On version 0.15 it was compiling or maybe using ts-node automatically to run the file.

elhigu commented 5 years ago

I just tested it... you are right, both versions are trying to register ts-node stuff. Sounds like that feature is not tested. Not sure if even documented.

elhigu commented 5 years ago

nope... now really tested it with necessary stuff installed on node 8

Mikaels-MacBook-Pro-2:test mikaelle$ npm install knex@0.15 ts-node-register
npm WARN test@1.0.0 No description
npm WARN test@1.0.0 No repository field.

+ knex@0.15.2
+ ts-node-register@1.0.0
updated 2 packages and audited 1037 packages in 2.135s
found 0 vulnerabilities

Mikaels-MacBook-Pro-2:test mikaelle$ echo "import * as knex from 'knex'; export = {client: 'pg'}" > knexfile.ts
Mikaels-MacBook-Pro-2:test mikaelle$ node_modules/.bin/knex migrate:make --knexfile knexfile.ts -x ts foo
Failed to load external module ts-node/register
Failed to load external module typescript-node/register
Failed to load external module typescript-register
Failed to load external module typescript-require
Failed to load external module @babel/register
/Users/mikaelle/Projects/Vincit/yessql/dodo-objection/test/knexfile.ts:1
(function (exports, require, module, __filename, __dirname) { import * as knex from 'knex'; export = {client: 'pg'}
                                                              ^^^^^^

SyntaxError: Unexpected token import

elhigu commented 5 years ago

cannot reproduce, but it is totally possible feature to be implemented, since knex already uses ts-node to compile actual typescript migration files and seeds

brunolm commented 5 years ago

I've setup a full example in case you want to investigate why it works on 0.15 and not on 0.16

Branch: knex16 has 0.16.3 installed https://github.com/brunolm/knex16bug/tree/knex16

Branch: knex15 has 0.15.2 installed https://github.com/brunolm/knex16bug/tree/knex15

Switch to the branch you want to check and run on the root folder:

docker-compose up

If it works you should see a migration called test in /api/src/db/migrations

elhigu commented 5 years ago

after installing also some more packages (ts-node ts-node-dev typescript) it seems that both, 0.15 and 0.16 works just fine here... but indeed your docker setup with node 0.16 is failing and I have no idea why

Mikaels-MacBook-Pro-2:test mikaelle$ rm -fr node_modules/*
Mikaels-MacBook-Pro-2:test mikaelle$ npm install knex typescript ts-node
npm WARN test@1.0.0 No description
npm WARN test@1.0.0 No repository field.

+ ts-node@7.0.1
+ knex@0.16.3
+ typescript@3.2.4
added 295 packages from 184 contributors and audited 1213 packages in 6.59s
found 0 vulnerabilities

Mikaels-MacBook-Pro-2:test mikaelle$ node_modules/.bin/knex migrate:make --knexfile knexfile.ts test
Requiring external module ts-node/register
Created Migration: /xxx/migrations/20190119003358_test.js
Mikaels-MacBook-Pro-2:test mikaelle$ cat knexfile.ts 
import * as knex from 'knex'; export = {client: 'pg'}
Mikaels-MacBook-Pro-2:test mikaelle$

I modified your docker-compose.yml to make pretty much the same and it fails like this:

version: '3'

services:
  api:
    image: node:8.14.0
    command: bash -c 'rm -fr api/node_modules; npm i knex ts-node typescript; node_modules/.bin/knex migrate:make --knexfile ./src/db/knexfile.ts test'
    working_dir: /usr/src/app
    volumes:
      - ./api:/usr/src/app

api_1  | npm WARN knex16@1.0.0 No repository field.
api_1  | 
api_1  | + ts-node@7.0.1
api_1  | + knex@0.16.3
api_1  | + typescript@3.2.4
api_1  | updated 3 packages and audited 1176 packages in 9.804s
api_1  | found 0 vulnerabilities
api_1  | 
api_1  | /usr/src/app/src/db/knexfile.ts:1
api_1  | (function (exports, require, module, __filename, __dirname) { import { database } from '../config'
api_1  |                                                               ^^^^^^
api_1  |

elhigu commented 5 years ago

If I moved knexfile in your container to /usr/src/app it did try to compile correctly... I cannot reproduce that behavior locally though. Here everything works even if knexfile.ts is inside subdirectories...

brunolm commented 5 years ago

I think I found maybe a bug on 0.16, or just something weird.

On 0.15 launch is called with configPath https://github.com/tgriesser/knex/blob/0.15.2/bin/cli.js#L260

But on 0.16 it's called with

    knexfile: argv.knexfile,
    knexpath: argv.knexpath,

https://github.com/tgriesser/knex/blob/0.16.3/bin/cli.js#L303-L304

LiftOff calls var env = this.buildEnvironment(opts); which calls findConfig({ passing configPath (which is no longer provided). Internally it uses configPath and makes no reference to knexfile or knexpath.

I have typescript and ts-node installed in the project and it works when I have v0.15 installed (like the example I provided on the Git repository.

I did some debugging and found this, but I still didn't figure out why it works on 0.15

`knex@0.15.2`

Logging the result of this line

var config = require(env.configPath);

https://github.com/tgriesser/knex/blob/0.15.2/bin/cli.js#L55

I get this

api_1    |   require(/usr/src/app/src/db/knexfile.ts)------------ { client: 'postgresql',
api_1    |   connection: 'postgres://user:password@db/api-db',
api_1    |   migrations:
api_1    |    { directory: '/usr/src/app/src/db/migrations1337',
api_1    |      tableName: 'knex_migrations' },
api_1    |   seeds: { directory: '/usr/src/app/src/db/seeds' } }
api_1    | this.config { extension: 'js',
api_1    |   loadExtensions:
api_1    |    [ '.co',
api_1    |      '.coffee',
api_1    |      '.eg',
api_1    |      '.iced',
api_1    |      '.js',
api_1    |      '.litcoffee',
api_1    |      '.ls',
api_1    |      '.ts' ],
api_1    |   tableName: 'knex_migrations',
api_1    |   schemaName: null,
api_1    |   directory: '/usr/src/app/src/db/migrations1337',
api_1    |   disableTransactions: false }

The migration directory is saying directory: '/usr/src/app/src/db/migrations1337',, which is exactly what I have on knexfile.ts

And it's directly requiring the file .ts

require('/usr/src/app/src/db/knexfile.ts')

`knex@0.16.3`

The require is to the exact same file, but it fails

// /usr/src/app/src/db/knexfile.ts
env.configuration = require(resolvedKnexfilePath);

brunolm commented 5 years ago

`knex@0.15.2`

I uninstalled typescript and ts-node. Now I get this:

api_1    | Failed to load external module ts-node/register
api_1    | Failed to load external module typescript-node/register
api_1    | Failed to load external module typescript-register
api_1    | Failed to load external module typescript-require
api_1    | Failed to load external module @babel/register

cli.on('requireFail', function(name) {
  console.log(chalk.red('Failed to load external module'), chalk.magenta(name));
});

https://github.com/tgriesser/knex/blob/0.15.2/bin/cli.js#L254

I don't see this message at all on 16, it just breaks.

brunolm commented 5 years ago

@elhigu I figured it out, I made a PR, can you check it? https://github.com/tgriesser/knex/pull/3005

elhigu commented 5 years ago

Reopening since this probably is a bug somewhere... even that I don't know how to reproduce it on osx.

scorbin commented 5 years ago

FYI, I ran brunolm's example and knex 0.15.2 works while knex 0.16.3 does not work

elhigu commented 5 years ago

@scorbin yes, I was able to reproduce that too with docker, but not locally on osx. That is why I reopened this.

pvoisin commented 5 years ago

Same problem here (TypeScript migrations + 0.16.3). When Migrator#latest is called, its starts with migrationListResolver#listAllAndCompleted which in turn calls listAll(config.migrationSource) where migrationSource having loadExtensions = [".co", ".coffee", ".eg", ".iced", ".js", ".litcoffee", ".ls", ".ts"], even if I { …, loadExtensions: ['.js'], … } in the migrator configuration. As a consequence, it picks both the .ts files and .js and tells the migration to go on with the .ts files (since they're told to be not applied yet, obviously), which... breaks when #getMigration is finally called to evaluate the script (since it calls #require with the migration script which is TypeScript).

Hope this helps...

With the following little change, things seem back to normal:

function listAllAndCompleted(config, trxOrKnex) {
  return _bluebird.default.all([listAll(config.migrationSource, config.loadExtensions), listCompleted(config.tableName, config.schemaName, trxOrKnex)]);
}

I didn't validate it is 100% compliant with the migrator's design though - one more knowledgeable than me would have to validate! :)

cham11ng commented 5 years ago

I was facing the same issue with Knex version 0.16.3.

import * as Knex from 'knex';
^^^^^^

SyntaxError: Unexpected token import
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:617:28)
    at Object.Module._extensions..js (module.js:664:10)

This solution is not encouraged but I simply fixed it by simply adding ts-node/register at the beginning of knexfile.ts file.

require('ts-node/register');
//

kibertoad commented 5 years ago

Should be fixed in 0.16.4-next2

juliancoleman commented 5 years ago

Just tried 0.16.4-next2. Sad to see the same error

elhigu commented 5 years ago

@brunolm can you confirm that 0.16.4-next2 still fails this?

kibertoad commented 5 years ago

@pvoisin Does it work for you with 0.16.4-next2?

FindAPattern commented 5 years ago

@kibertoad I'm getting the exact same error on 0.16.4-next2.

kibertoad commented 5 years ago

OK, so the proper fix for this would be to fully remove dependency on knexfile from migration:make (since it not really needs it).

juliancoleman commented 5 years ago

I don't want to beat the dead horse, but @kibertoad said it should be fixed. Where was the evidence of this fix? Have regression tests been written? Saying it should be fixed, closing the issue, and having the same issue present is not the way releases should be pushed.

elhigu commented 5 years ago

@juliancoleman this has been difficult to reproduce. But indeed there should have been regression test added in the fix (didnt check if there actually was and test just didnt fail on ci env).

juliancoleman commented 5 years ago

Difficult to reproduce is actually fair as I've looked back and have seen that no one, including myself, has added steps to reproduce.

Add knex@0.16.4-next2 to dependencies
Create a knexfile.ts
(optionally using ObjectionJS) instantiate knex config on Model.knex
Add the following script to package.json
- "knex": "knex --knexfile ./path/to/Knexfile.ts"
Make migrations
- yarn knex migrate:latest
Run into SyntaxError

juliancoleman commented 5 years ago

I've verified these steps on a project I just started the other day, which brought me to this issue

brunolm commented 5 years ago

    "knex": "^0.16.4-next2",

git clone git@github.com:brunolm/knex16bug.git
cd knex16bug
git checkout knex16
npm run api-i
docker-compose up

api_1    | > knex migrate:make --knexfile ./src/db/knexfile.ts "test"
api_1    |
api_1    | /usr/src/app/src/db/knexfile.ts:1
api_1    | (function (exports, require, module, __filename, __dirname) { import { database } from '../config'
api_1    |                                                               ^^^^^^
api_1    |
api_1    | SyntaxError: Unexpected token import
api_1    |     at createScript (vm.js:80:10)
api_1    |     at Object.runInThisContext (vm.js:139:10)
api_1    |     at Module._compile (module.js:617:28)
api_1    |     at Object.Module._extensions..js (module.js:664:10)
api_1    |     at Module.load (module.js:566:32)
api_1    |     at tryModuleLoad (module.js:506:12)
api_1    |     at Function.Module._load (module.js:498:3)
api_1    |     at Module.require (module.js:597:17)
api_1    |     at require (internal/module.js:11:18)
api_1    |     at initKnex (/usr/src/app/node_modules/knex/bin/cli.js:62:25)
api_1    |     at Command.commander.command.description.option.action (/usr/src/app/node_modules/knex/bin/cli.js:172:24)
api_1    |     at Command.listener (/usr/src/app/node_modules/commander/index.js:315:8)
api_1    |     at emitTwo (events.js:126:13)
api_1    |     at Command.emit (events.js:214:7)
api_1    |     at Command.parseArgs (/usr/src/app/node_modules/commander/index.js:654:12)
api_1    |     at Command.parse (/usr/src/app/node_modules/commander/index.js:474:21)
api_1    | npm ERR! code ELIFECYCLE
api_1    | npm ERR! errno 1
api_1    | npm ERR! knex16@1.0.0 migrate-make: `knex migrate:make --knexfile ./src/db/knexfile.ts "test"`
api_1    | npm ERR! Exit status 1
api_1    | npm ERR!
api_1    | npm ERR! Failed at the knex16@1.0.0 migrate-make script.
api_1    | npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
api_1    |
api_1    | npm ERR! A complete log of this run can be found in:
api_1    | npm ERR!     /root/.npm/_logs/2019-03-18T21_39_10_408Z-debug.log

My PR fixes it, I just don't have the knowledge to fully test it etc, if someone could look into it...

https://github.com/tgriesser/knex/pull/3005

kibertoad commented 5 years ago

I don't want to beat the dead horse, but @kibertoad said it should be fixed. Where was the evidence of this fix? Have regression tests been written? Saying it should be fixed, closing the issue, and having the same issue present is not the way releases should be pushed.

I apologize, it was a late hour here in Amsterdam and my choice of words was really not the best. It should have been "Please let me know if 0.16.4-next2 fixes the problem", as, TBH, I would have been very surprised if it did. There was a unit test written that verifies exactly what this fix actually fixes, and now I'm fairly sure that I'm not misunderstanding anything and that problem is exactly what I thought it is, and the only way how it could have worked previously is through bypassing knexfile entirely. This is a system behaviour that I intend to improve in the nearby future, but it requires a bit of prerequisites to be delivered before. I apologize for the inconvenience. Just give me some time. In the meantime, using ts-node looks to be a proper solution - one might argue that it's actually less hacky than not depending on knexfile, since it allows you to use knexfile as a single source of truth regarding the location of migration files.

brunolm commented 5 years ago

@kibertoad

and the only way how it could have worked previously is through bypassing knexfile entirely.

This is not true, see my previous comments. Everything works on 0.15.

kibertoad commented 5 years ago

@brunolm But can you explain on a technical level how Node.js may not choke while parsing Typescript file even hypothetically? I'll check the PR out, but I suspect that it achieves same thing ultimately (although by different means) - bypasses the knexfile.

juliancoleman commented 5 years ago

@kibertoad I am presently using ts-node. I'm not sure why this happens on a technical level

juliancoleman commented 5 years ago

If I had the time to check the source, I would, but I've been using Knex for years and I just trust it works at this point. I reverted back to 0.15 and everything works... Except this, of course

brunolm commented 5 years ago

@brunolm But can you explain on a technical level how Node.js may not choke while parsing Typescript file even hypothetically? I'll check the PR out, but I suspect that it achieves same thing ultimately (although by different means) - bypasses the knexfile.

if you do this you can see it fully working on knex 0.15, not ignoring the knexfile.ts file

git clone git@github.com:brunolm/knex16bug.git
cd knex16bug
git checkout knex15
npm run api-i
docker-compose up

however if you run the exact same thing on knex 0.16 you will see the error. If you clone this I suggest cloning in a different folder so you can make sure you're on the right version

git clone git@github.com:brunolm/knex16bug.git
cd knex16bug
git checkout knex16
npm run api-i
docker-compose up

elhigu commented 5 years ago

I verified those examples of @brunolm earlier I was never able to create reproduction case for this that would fail on macos, but I was able to reproduce docker setup.

So the problem currently would be to be able to create test case for knex CI, which actually also fails on 0.16 and works with 0.15. Since CI is running some old ubuntu, maybe that test case works (meaning fails) there just fine.

FindAPattern commented 5 years ago

Quick update on my end. Not sure if this is useful or not, but my issue was that I was using multiple tsconfig.json files in my project, and knex was using the default. When I overrode this functionality by directly launching knex using ts-node, I was able to get around this error.

My updated package.json:

// ...
"knex": "ts-node --project tsconfig.server.json $(npm bin)/knex --knexfile ./src/server/knexfile.ts",
// ...

How I run migrations now:

npm run knex -- migrate:latest

Hopefully this helps someone.

juliancoleman commented 5 years ago

@FindAPattern would you mind posting your tsconfig? Here's mine. I only use one, since I have no need to build as I serve with ts-node

{
  "compilerOptions": {
    "noEmit": true,
    "rootDir": "src",
    "target": "es6",
    "module": "commonjs",
    "moduleResolution": "node",
    "declaration": true,
    "inlineSourceMap": true,
    "resolveJsonModule": true,
  },
  "include": [
    "src/**/*.ts",
  ],
  "exclude": [
    "src/**/*.spec.ts"
  ]
}

antoniogiroz commented 5 years ago

Hi! Is there any progress on this?

@brunolm How are you solve it? Any workaround?

brunolm commented 5 years ago

@algil I didn't need to update or start a new project yet, so I'm still using 0.15 for the most part.

But I did make a PR that solves this issue, You can fork knex and apply these changes to it: https://github.com/tgriesser/knex/pull/3005

kibertoad commented 5 years ago

It's not so simple, unfortunately. I'll try to come up with a fix that doesn't break any use-cases this week.

a-resh commented 5 years ago

Maybe it will help someone. Fixed when change in tsconfig.json "module": "es2015" to "module": "commonjs" Command in package.json => "migrate": "ts-node -P ./tsconfig.json node_modules/.bin/knex migrate:latest --knexfile knexfile.ts".

Meldiron commented 5 years ago

Maybe it will help someone. Fixed when change in tsconfig.json "module": "es2015" to "module": "commonjs" Command in package.json => "migrate": "ts-node -P ./tsconfig.json node_modules/.bin/knex migrate:latest --knexfile knexfile.ts".

Not optimal solution but works fine.

juliancoleman commented 5 years ago

@Areshetcov @Meldiron I've been using commonjs module as you can see above. I think what really needs to happen is a deeper support, not necessarily causing a ton of user configuration. Unless that truly is not an option

juliancoleman commented 5 years ago

Considering how this issue is related to a release that's now behind, is this currently an issue with 0.17.6?

kibertoad commented 5 years ago

@juliancoleman Using ts-node is still the best solution, as knex still tries to open knexfile.js and explodes if it is not in Javascript format. There were improvement to handle TS extensions more gracefully in more cases (e. g. migration generation or resolving knexfile in default location), however. Which particular issue are you having?

kibertoad commented 5 years ago

In any case, thank you for reminding me about this issue. I'll try to take another swing at it on the weekend, hopefully I can at least understand why it worked in the past :D

knex / knex

knex 0.16 doesn't support knexfiles written in TypeScript #2998

Environment

Bug

`knex@0.15.2`

`knex@0.16.3`

`knex@0.15.2`