graphile / migrate

Opinionated SQL-powered productive roll-forward migration tool for PostgreSQL.
MIT License
751 stars 58 forks source link

fix: Use .default to access CJS module for dynamic imports #202

Closed scottsidwell closed 10 months ago

scottsidwell commented 10 months ago

Description

Small fix to address a regression introduced as part of #198, when we moved from require -> import.

When dynamic-importing CJS/ESM modules, export default { ... } and module.exports = ... is placed under a default key (as opposed to require where the CJS module was returned as the top level object. This meant using a .cjs config would fail when parsing the settings with:

Error: Errors occurred during settings validation:
- Setting 'connectionString': Expected a string, or for DATABASE_URL envvar to be set
- Setting 'databaseOwner': Expected a string or for user or database name to be specified in connectionString
- Setting 'shadowConnectionString': Expected `shadowConnectionString` to be a string, or for SHADOW_DATABASE_URL to be set
- The following config settings were not understood: 'default'
- Could not determine the shadow database name, please ensure shadowConnectionString includes the database name.

I explored adding regression tests to ensure this didn't happen again but it looks like jest needs some extra configuration to support dynamic import()s so I've opted for the fingers-crossed🤞 approach that we won't accidentally break this again instead. If you reckon there's a clear approach to support this in jest, happy to add as part of the PR.

Checklist

benjie commented 10 months ago

Fix released in graphile-migrate@2.0.0-rc.2 :raised_hands: