node-pg-migrate icon indicating copy to clipboard operation
node-pg-migrate copied to clipboard

node-pg-migrate cannot load migrations that are in upstream-only directories

Open leinelissen opened this issue 3 years ago • 2 comments

Hi there,

I am currently working on a legacy codebase which implements node-pg-migrate. It basically does a directory search in a particular folder, and then passes those files one-by-one into the runner programmatic API. I ran into a really weird issue where a directory structure like this would succeed in running the migrations:

.
├ src/
│ └ migrations.js
└ migrations/
  ├ migration1.ts
  ├ migration2.ts
  └ ...

whereas the following directory structure would result in an Error: Cannot find module 'migrations/migrations1.ts'

.
├ migrations.js
└ migrations/
  ├ migration1.ts
  ├ migration2.ts
  └ ...

I dove down the rabbit hole and uncovered the following line that is at the basis of the issue:

https://github.com/salsita/node-pg-migrate/blob/9331f6fda98795e3f3733461f9b71345168b99d8/src/runner.ts#L35

What it comes down is the following. As per the NodeJS module resolution algorithm, only specific cases are resolved to local files. When path.relative calculates a path that has to move downstream first before moving upstream (Example 1), the paths it produces are always prefixed with ../. In this case the module resolution algorithm will assume the imports are local files, resolve the paths and them import them. The issue comes with Example 2. If the path is straight upstream from the file itself, path.relative doesn't have to prefix the path with anything, thus resulting in a relative path like migrations/migrations1.ts. However, because the lack of prefix, the resolution algorithm will now recognise the import as a module import, which has many different rules for import, of which none is importing local files. Hence the import will fail.

There are two solutions for resolving these issues:

  1. Making sure that if the relative path is not prefixed with ../ or..\, to prefix it with ./ or .\ so Node knows to resolve the path as a local file
  2. Forgoing path.relative completely, and just directly inputting the absolute path, which Node will resolve as local file as well.

I don't necessarily see why path.relative was necessary in the first case, but I can imagine that the first solution is more backwards-compatible regarding edge cases. I would love to make a PR to fix this issue, but I need some input first as to which solution makes more sense.

Cheers! Lei

leinelissen avatar Jul 22 '22 08:07 leinelissen

I think this is another issue that could benefit from #651

moltar avatar Oct 18 '22 15:10 moltar