fix(esm): __dirname and pg.Client
#1206
Conversation
aprendendofelipe
changed the title
__dirname, pg.Client and types__dirname, pg.Client and types
| // @ts-expect-error: ignore until esm only | ||
| import.meta.url || __dirname | ||
| ); | ||
| const crossRequire = createRequire(resolve('noop.js')); |
There was a problem hiding this comment.
question (non-blocking): what is this noop.js? I don't know this file 🤔
There was a problem hiding this comment.
no op = no operation
It is a file that does not exist and will be ignored. Only the path matters.
The same hack is used internally in module.createRequire:
const proxyPath = trailingSlash ?
path.join(filename, 'noop.js') :
filename; There was a problem hiding this comment.
Interesting 🤔
I first thought you wanted to reference somehow https://github.com/salsita/node-pg-migrate/blob/main/test/migrations/001_noop.js, but it did not make any sense to me.
- I need to check how this behaves. Why does it not error when a file is not found/invalid. And what is the fallback behavior instead.
- If we are really using this, we should extract it into a
const NON_EXISTING_FILE = 'noop.js'or similar, and potentially even add JSDoc to that constant.
There was a problem hiding this comment.
I made a small test to see how noop.js behaves 👀
❯ node index.js
metaUrl: file:///Users/cquadflieg/WPSM/wui/test-project/index.js
noopUrl: /Users/cquadflieg/WPSM/wui/test-project/noop.js
❯
❯ node src/index.js
metaUrl: file:///Users/cquadflieg/WPSM/wui/test-project/src/index.js
noopUrl: /Users/cquadflieg/WPSM/wui/test-project/noop.js
❯
❯ cd src
❯ node index.js
metaUrl: file:///Users/cquadflieg/WPSM/wui/test-project/src/index.js
noopUrl: /Users/cquadflieg/WPSM/wui/test-project/src/noop.js
❯
❯ node ../index.js
metaUrl: file:///Users/cquadflieg/WPSM/wui/test-project/index.js
noopUrl: /Users/cquadflieg/WPSM/wui/test-project/src/noop.jsso..., import.meta.url is always pointing to the file where the statement is written in.
But noop is always the path where the execution was made from. (is it similar to process.cwd()?)
There was a problem hiding this comment.
The file name does not need to be noop.js. We can use any name as it will be ignored by Module.createRequire. In the end, only the directory matters.
process.cwd() will not work, as dirname(process.cwd()) will resolve to a level higher than desired.
It could be `${process.cwd()}/` because internally Module.createRequire will join with noop.js:
const trailingSlash =
StringPrototypeEndsWith(filename, '/') ||
(isWindows && StringPrototypeEndsWith(filename, '\\'));
const proxyPath = trailingSlash ?
path.join(filename, 'noop.js') :
filename;And it should work fine on Windows too.
But I prefer something more flexible, like resolve(process.cwd(), 'anything.js'), in which case, due to the internal workings of resolve, we can omit process.cwd(). That's why I'm proposing something like resolve('NON_EXISTING_FILE_OR_FOLDER') or resolve('_'), but it can be whatever you prefer, as the result will be the same:
createRequire(resolve('NON_EXISTING_FILE_OR_FOLDER'))createRequire(`${process.cwd()}/`)createRequire(resolve('_'))- ...
There was a problem hiding this comment.
looks like even that does not work
There was a problem hiding this comment.
I tried several other things locally
Could you try again on your own to bring the test\runner.spec.ts green, without touching it?
| @@ -16,7 +16,7 @@ type Options = | |||
| export const run = async (options: Options): Promise<boolean> => { | |||
| const opts: Omit<RunnerOption, 'direction'> & Options = { | |||
| migrationsTable: 'migrations', | |||
| dir: resolve(__dirname, 'migrations'), | |||
| dir: resolve('test', 'ts', 'migrations'), | |||
There was a problem hiding this comment.
thought (if-minor): this is just a test-file and does not need to be changes until package.json is set to "type": "module"
There was a problem hiding this comment.
True, but we are already trying to remove everything that depends on __dirname. I don't see why leaving it for later, but I can remove the change if you think it's better. 🤝
There was a problem hiding this comment.
It is more like putting focus on one thing (fixing the source code for ESM).
But I might see now what you did here.
So... I follow the strategy to squash-merge every PR and using its title as commit message, so maybe extract this change into its own PR with title test(runner): <message>
| const actions: MigrationBuilderActions = | ||
| extname(filePath) === '.sql' | ||
| ? await migrateSqlFile(filePath) | ||
| : require(relative(__dirname, filePath)); | ||
| : createRequire(resolve('noop.js'))(filePath); |
There was a problem hiding this comment.
question (non-blocking): is this a leftover? or did you not rebase on the latest main-branch? 👀
If this is still in the main-branch this indeed needs to be fixed.
Could you extract this out into its own PR please?
There was a problem hiding this comment.
This branch is up to date.
I can extract this in a separate PR, but removing the last commit should be enough. Or not?
There was a problem hiding this comment.
Yes, we now use this (or #1207) as the PR that will fix the esm source code issue
I really appreciate your help and I'm learning new things, but as I wrote in my other comment regarding the squash-merge PRs, try to make dedicated PRs
| 'templates', | ||
| `migration-template.${await resolveSuffix(directory, options)}` | ||
| ) | ||
| 'node_modules/node-pg-migrate/templates', |
There was a problem hiding this comment.
question (if-minor): does this work in pnpm as well?
why is this change better than the other?
There was a problem hiding this comment.
It does not depend on tsup handling import.meta.url and __dirname
There was a problem hiding this comment.
Good point, I will look into this later. I'm just only afraid of this code-piece in relation to several package-managers (yarn, pnpm, bun, and so on). e.g. yarn could have something so that there isn't even a node_modules folder, is it? (I'm not working personally with yarn anymore)
| // This needs to be imported as `*`, otherwise it will fail in esm | ||
| import * as pg from 'pg'; |
There was a problem hiding this comment.
question (if-minor): there is an explicit comment that esm would fail 🤔
you sure that this is correct? why would it suddenly work?
There was a problem hiding this comment.
Run node bin/node-pg-migrate.mjs up (mjs) and you will get TypeError: pg.Client is not a constructor instead of the connection error you receive with node bin/node-pg-migrate.js up (js).
With import pg from 'pg' you will only see the connection error in both cases, which is as expected
There was a problem hiding this comment.
I will look into this again later.
Maybe this should be extracted into its own PR.
aprendendofelipe
changed the title
__dirname, pg.Client and types__dirname and pg.Client
|
Two more things:
|
Resolve #1167 :
__dirname is not definedResolve #1190 :
pg.Client is not a constructorIt is possible to test with
node-pg-migrate-exp@7.4.0-experimental-1189-13:npm i node-pg-migrate-exp