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

Allow both ts and js files to run migrations #715

Closed
wants to merge 1 commit into from

Conversation

orta
Copy link

@orta orta commented Nov 9, 2018

Because you can use ts-node node_modules/.bin/sequelize db:migrate to trigger your migrations, you can safely use TypeScript using more or less vanilla sequelize-cli. The only blocker I've found so far is the check for js files in the migration file detection.

This is great, because you get awesome tooling support

screen shot 2018-11-08 at 7 14 16 pm

This PR allows a .ts file as well as a .js to pass by default.

> "hello.ts".match(/\.(t|j)s$/)
< [".ts", "t"] (2)
> "hello.js".match(/\.(t|j)s$/)
< [".js", "j"] (2)
> "hello.coffee".match(/\.(t|j)s$/)
< null

There's a downside to this in that someone could try running a typescript file without the ts-node, but this app is not documented as supporting typescript out of the box, so I don't think people would have that expectation.

@markcellus
Copy link

Just ran into the File does not match pattern: /\.js$/ error when attempting to run the seed and migrate commands using ts-node-dev. This should fix that, so thank you!!!

Will this change also work for the seed command and when running a ts file as the config?

sequelize db:seed:all --config=./config/sequelize-config.ts

@markcellus
Copy link

Also look likes this would resolve #693, if merged. Hopefully this would be accepted soon.

@markcellus
Copy link

@orta did you create a fork of this that can be used until this PR is merged? Really could use this. This is a very tiny change that is fully backwards-compatible so not sure why it's been sitting for so long.

@orta
Copy link
Author

orta commented Dec 3, 2018

Afraid not, I used patch-package which I use for any trivial PR change, it's very easy to use

@xianxingg
Copy link

This is a temp solution.
If you want to use typescript, use it from the beginning to the end. That's to say, the option --typescript should be supported for generating migration files, and cli should do the job of creating ".ts" file with "export default = { ... }" as content.

@ghost
Copy link

ghost commented Mar 25, 2019

Hi I'd like to ask when the PR will be accept ? It is quite a while :/. I tried this fix and it look that is working very well

@TranBaVinhSon
Copy link

Hmm, why this PR still not merged?

@papb
Copy link
Member

papb commented Aug 6, 2019

Hi, us maintainers have been focusing on the main repo recently, but I intend to take a good look on the CLI in the near future. Thanks everyone for the patience!

@brianemoore57
Copy link

One possiblity is to allow the user to add his own check for js files or globbing expression. A related issue would be the ability to suppress the 'does not match pattern: /.js$/ We have a large project and wish to use subdirectories like /migrations/data and seed/data but it results in a lot of distracting messages.

@max10rogerio
Copy link

max10rogerio commented Dec 5, 2019

I forked this repository and tried this solution! And works!

I just added the script on package:
"sequelize-cli-ts": "node -r ts-node/register ./node_modules/sequelize-cli/lib/sequelize"

example

import { QueryInterface, DataTypes, literal } from 'sequelize'

export function up(queryInterface: QueryInterface): Promise<void> {
  return queryInterface.createTable('Estados', {
    id: {
      allowNull: false,
      autoIncrement: true,
      type: DataTypes.INTEGER,
      comment: 'Identificador da tabela Estados',
      primaryKey: true,
    },
    nome: {
      allowNull: false,
      type: DataTypes.STRING(20),
      comment: 'Nome do estado',
    },
    sigla: {
      allowNull: false,
      type: DataTypes.STRING(2),
      comment: 'Sigla do Estado',
    },
    createdAt: {
      allowNull: false,
      type: DataTypes.DATE,
      comment: 'Data da criação do registro',
      defaultValue: literal('NOW()'),
    },
    updatedAt: {
      allowNull: false,
      type: DataTypes.DATE,
      comment: 'Data da última alteração do registro',
      defaultValue: literal('NOW()'),
    },
    deletedAt: {
      allowNull: true,
      type: DataTypes.DATE,
      comment: 'Data da exclusão do registro',
    },
  })
}

export function down(queryInterface: QueryInterface): Promise<void> {
  return queryInterface.dropTable('Estados')
}

run with: yarn sequelize-cli-ts db:migrate

@cjancsar
Copy link

@papb can this be merged?

@ismail-codinglab
Copy link

All checks passed, no conflicts

why not merge?

@ismail-codinglab
Copy link

Looking at the main contributors

@sdepold @sushantdhiman @Americas

@kf6kjg
Copy link

kf6kjg commented Feb 26, 2020

The problem I would have with this commit is that it allows the following files to be considered different migrations:

fixstage.ts
fixstage.js

To me those are the same migration, just one was compiled from the other. Compiling the TS into the same folder is indeed very bad form, but it still is a logical problem.

Also I'd personally, though I'm by no means a maintainer of this project, that a PR exist for adding a flag that allows the user to specify the matching pattern or file extension with the default left as it stands in the project currently. That way the user can override but the default is left unaffected.

Lastly, consider the principle of least surprise: which is worse, finding that after you upgraded your tools that all of a sudden both the TS and JS migrations files are being executed and you are being hit with failures left and right, or that an option now exists to change the pattern so that your TS files are run? I'd argue the latter is the least surprising and thus better.

@namnm
Copy link

namnm commented Apr 7, 2020

I have been able to work around this using a postinstall script to run this snippet:

const { readFileSync, writeFileSync } = require('fs')
const { join } = require('path')

// Fix sequelize-cli doesnt support typescript extensions
// Will be run in the postinstall script in package.json
const p = join(__dirname, '../node_modules/sequelize-cli/lib/core/migrator.js')
const f = readFileSync(p, 'utf8').replace('.js$/', '.(j|t)sx?$/')
writeFileSync(p, f, 'utf8')

@mohsen1
Copy link

mohsen1 commented Apr 20, 2020

@namnm patch-package npm package is a more clean hacky solution

@psi-4ward
Copy link

push bump +1 :)

@psi-4ward
Copy link

So the best option would be a config setting in .sequelizerc !

@kf6kjg
Copy link

kf6kjg commented May 8, 2020

I switched my project over to Umzug, which is the sequelize library that this project uses to do migrations. Gave me a lot more fine grained control over the process and allowed for my application to run migrations on startup.
A recent addition to Umzug solves the problem that this PR attempts by simply making the file extension not part of the comparison and not stored.

@MBerka
Copy link

MBerka commented May 9, 2020

TS migration blues led me to the same decision earlier today. In case anyone else comes looking for a way to get a ts-node Mocha integration test to load their TS migrations despite those migrations having no JS counterparts during testing because ts-node does not output files and it's all buried in a container - the simplest approach with umzug uses an arbitrary pattern parameter, { pattern: /^\d+[\w-]+\.(js|ts)$/ }.
That's a more rigorous pattern too; there could be multiple use cases to keep the default "js" pattern, and expose an optional pattern input, not just to admit TS, but to block arbitrary other files. This being CLI, maybe as a string passed to RegExp()? It's a heavier change though.

@markcellus
Copy link

markcellus commented May 31, 2020

I switched my project over to Umzug

@kf6kjg This is an option but AFAIK I don't think umzug does seeding out of the box like this library does? You could do some manual work to get umzug to work with seeding as evidence by this comment, but its a brittle workaround with a few caveats.

@kf6kjg
Copy link

kf6kjg commented May 31, 2020

@mkay581 in Umzug seeders are just another set of migrations. You are totally in control of how you want to structure all parts of your migration: you could have schema migrations, seeds, data migrations, and any other migration type your project needs just by running Umzug against either different folders, different file name patterns, or both.

Of course Umzug isn’t a cli interface, but I really didn’t need a cli interface in my projects.

@markcellus
Copy link

markcellus commented May 31, 2020

Sure, although some people would rather not have to worry about such granular level of detail, which is why people choose to use this higher level library.

@ismail-codinglab
Copy link

If you would ask me what would I do different if I would start a typescript project again is that I would use typeorm. Sequelize isn't build for typescript, it's Javascript conversion to typescript

@markcellus
Copy link

it's Javascript conversion to typescript

JavaScript doesnt get converted to TypeScript

@MBerka
Copy link

MBerka commented Jun 1, 2020

@mkay581 Not automatically, no, only when you start with a plain JS project and later manually move the project to TS, migrations and all, learning all the typing tricks needed to make Sequelize keep working along the way. I think that's the point - that TypeORM is TS-native.
That said, I was interested to find sequelize-typescript that makes Sequelize's models look rather similar.

@kroleg
Copy link
Contributor

kroleg commented Jun 25, 2020

@sushantdhiman can we please include this in v6? It's a super easy and long awaited change. And cjs support was just added.

@kroleg
Copy link
Contributor

kroleg commented Jul 3, 2020

@sushantdhiman can you please explain why did you close this one?

@orta
Copy link
Author

orta commented Jul 3, 2020

Screen Shot 2020-07-03 at 10 55 15 AM

All PRs were closed ^

@sushantdhiman
Copy link
Contributor

PR Cleanup, if anyone needs this feature they should open a new PR & add a test for this. For example #905

@yonycalsin
Copy link

npx ts-node -T -r tsconfig-paths/register .\node_modules\sequelize-cli\lib\sequelize db:migrate

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

Successfully merging this pull request may close these issues.

None yet