fixes orm migrations on start up

[matt-raffel-kiva]

Signed-off-by: Matt Raffel mattr@kiva.org

[jsaur]

Did you change something in the package.json? Just saw that 12K lines where removed from package-lock.json so wondering what's going on

[matt-raffel-kiva]

Did you change something in the package.json? Just saw that 12K lines where removed from package-lock.json so wondering what's going on

no. after running npm install, it changed. I assumed the previous package-lock file was wrong. I can revert if we wish.

[ghost]

What's the problem this is fixing? As far as I can tell, the code works as intended

Here's an example branch with a migration that works every way I can test it: https://github.com/kiva/aries-guardianship-agency/tree/sample-migration

[matt-raffel-kiva]

What's the problem this is fixing? As far as I can tell, the code works as intended

Tables are not getting created when:
1 - running aries-guardianship-agency\docker-compose.yml without calling npm run build locally first (this is the first element in the orm config arrays for migrations (and enitities)
2 - running protocol-aries\docker-compose.dep.yml (this is the second element)

[ghost]

without calling npm run build locally first

I know we talked about this elsewhere already, but this is the problem.

running protocol-aries\docker-compose.dep.yml

Which part is the fix for this? Is it the entities?

[matt-raffel-kiva]

without calling npm run build locally first

I know we talked about this elsewhere already, but this is the problem.

running protocol-aries\docker-compose.dep.yml

Which part is the fix for this? Is it the entities?

We have already discussed all of this in slack. I am really confused why we are going over this again. If you would like, please work with Cam to see the problems. I am tired of being the middle man on this.

[ghost]

I am tired of being the middle man on this.

This is why I brought it up in public, so that you don't have to be the middle man. I was hoping someone else, who is facing the issue, can explain to me what is going on now that it's in public. I'm definitely not trying to pull you in again.

At the same time, I'm not very comfortable with this getting merged based on what I've heard about the problem, so I want my objection noted on the PR so others understand why it hasn't been merged yet.

[jsaur]

This looks good to me 👍
FYI @jeffk-kiva it's nice when references to entities and migrations work both with typescript and with javascript as done here. It's good for developers to run build to transpile to js but it shouldn't be required (and ci runs build too so no worries there).

Note: the migration issue on my mac was I hadn't copied dummy.env to .env recently. I can confirm that this PR runs migrations on my machine even without a dist folder 👍

[ghost]

FYI @jeffk-kiva it's nice when references to entities and migrations work both with typescript and with javascript as done here.

@jsaur The problem is if you have a reference to the same migration in both typescript and javascript, your code will fail, and it will do so in a fairly opaque way. This effectively forces you to use typescript locally.

It's good for developers to run build to transpile to js but it shouldn't be required (and ci runs build too so no worries there).

Agree to disagree I guess, but there is absolutely no reason to skip the transpilation phase. Especially if you're working on code that alters core infrastructure, why would anyone be comfortable testing code that isn't the actual code that will be run in other environments? If it's because it's annoying to type one extra command, then just alter the npm run start alias to include the build step. By the same argument, there's no reason for developers to run tests because CI runs tests. Surely you're not suggesting that, though.

All the "hard" problems we face using typescript are because it's actually javascript underneath the surface. When you decide to skip the transpilation step, you're saying you'd rather deal with those problems later in a remote environment, rather than sooner in a local environment. Do we suddenly not value "failing fast"?

Also, you'll get errors that aren't actually errors. For example, reflect-metadata requires being run on transpiled javascript. So that means that the runtime validations from protocol-common won't work, in addition to other dependencies brought in by NestJS that aren't under our control.

I would agree with you if we were using ts-node in production (which apparently is mostly safe to do: TypeStrong/ts-node#104), but until then, I can't.

the migration issue on my mac was I hadn't copied dummy.env to .env recently.

So the problem was not with migrations, but rather with environment variables. Why did we merge a "fix" when there's nothing to fix?

[matt-raffel-kiva]

So the problem was not with migrations, but rather with environment variables. Why did we merge a "fix" when there's nothing to fix?

It did fix an issue, Jeff.

[ghost]

It did fix an issue, Jeff.

What issue did it fix? And to be clear, not being able to run migrations without transpiling your code is not an issue.

[jsaur]

I think my point is that philosophically we want things to work for developers when their developing locally using typescript and ts-node AND we also want things to work in javascript in production. For this particular case I didn't know that "if you have a reference to the same migration in both typescript and javascript, your code will fail, and it will do so in a fairly opaque way" - could you explain a bit more? If that's the case, then I agree, it's a good reason to say only do one or the other, and since we're using js in production we should do that. For most other things, eg Typeorm entities, it's really nice to have paths defined for both ts and js.

[ghost]

philosophically we want things to work for developers when their developing locally using typescript and ts-node AND we also want things to work in javascript in production.

While that may be nice in some cases, I don't really see the value. The only argument I can see for running ts-node is that some people may simply prefer it because they can add breakpoints more easily if they're debugging typescript. IDEs make that point moot, but I suppose we want to support the vim and emacs users of the world too. The argument that it's "easier" to run is also wrong, because all you have to do is change the npm start alias to include a build step.

What supporting both typescript and javascript really means is doubling the work, because to be sure everything actually works, it has to be tested and developed in typescript and in javascript. I'm sure that most developers are too lazy to do that, which means we have to rely on CI, which can be opaque and confusing when it fails. In other words, it makes it much harder to fail fast and visibly. There are many examples of this, one of which is the duplicate migration issue. Anyway, my point is that philosophically we want our code to work reliably, and choosing to run two different sets of code introduces a lot of complexity, which makes code not very reliable. What works for one developer will now not work for another developer.

I get that there's often a tradeoff between making things as easy as possible for the developer and having reliable code. We want to be open and support a community, so we need to value making things easy for the developer. However, if the code is failing in ways that are invisible to some developers, that's not making things easy. And when it comes to code that manages the database, I think having reliable code is far more important than having "easy" code.

For this particular case I didn't know that "if you have a reference to the same migration in both typescript and javascript, your code will fail, and it will do so in a fairly opaque way" - could you explain a bit more?

With the changes from this branch, if I run npm run build && npm run start locally, then aries-guardianship-agency fails to start and I see this error:

[Nest] 50618   - 06/15/2021, 8:20:39 AM   [TypeOrmModule] Unable to connect to the database. Retrying (1)... +439ms
Error: Duplicate migrations: AgentTransactions1620304611265, CreateFoo1623428871238
    at MigrationExecutor.checkForDuplicateMigrations (/Users/jeffk/src/kiva/aries-guardianship-agency/src/migration/MigrationExecutor.ts:424:19)
    at MigrationExecutor.getMigrations (/Users/jeffk/src/kiva/aries-guardianship-agency/src/migration/MigrationExecutor.ts:414:14)
    at MigrationExecutor.<anonymous> (/Users/jeffk/src/kiva/aries-guardianship-agency/src/migration/MigrationExecutor.ts:175:36)
    at step (/Users/jeffk/src/kiva/aries-guardianship-agency/node_modules/typeorm/node_modules/tslib/tslib.js:143:27)
    at Object.next (/Users/jeffk/src/kiva/aries-guardianship-agency/node_modules/typeorm/node_modules/tslib/tslib.js:124:57)
    at fulfilled (/Users/jeffk/src/kiva/aries-guardianship-agency/node_modules/typeorm/node_modules/tslib/tslib.js:114:62)

For most other things, eg Typeorm entities, it's really nice to have paths defined for both ts and js.

I'm fine with this, I just don't understand what problem it solves, or why it's nice in this case.

[jsaur]

Ok so it sounds like there's a general issue and a specific issue:
General issue: Why use ts-node at all? My initial hypothesis was that this would make development faster, but it may not be true. If it's not then I agree it overcomplicates things to have to worry about both ts and js paths. I created this ticket https://kiva.atlassian.net/browse/PRO-3130 (and assigned to you :) ) to spike out one of our repos removing ts-node.
Specific issue: These migrations. If this code change causes things to break on our dev envs or prod then let's revert it. However, I'm unable to reproduce your issue (what's CreateFoo?). If it's not breaking anything right away, and we got with the long term strategy of removing ts-node (above), then we can remove the ts paths when we remove ts-node

[ghost]

That sounds like a solid plan. And if it ends up being more of a pain than it's worth, then we can ignore it.

CreateFoo came from the branch I made demonstrating a migration. I can walk you through how to reproduce it, but it might be easier on a call. There could be a bunch of reasons why you weren't able to reproduce it, cache invalidation being foremost amongst them, which is why I prefer not to develop in docker.

But also, to be clear, I'm not saying this code breaks anything in CI or production, because the production Dockerfile doesn't copy anything from src and therefore only the .js is available in those environments. It's only local development where things are tricky.