worker icon indicating copy to clipboard operation
worker copied to clipboard

Improve webpack-ability of worker

Open joelmukuthu opened this issue 5 years ago • 4 comments

Feature description

It would be nice to be able to specify an option that tells Graphile worker to not attempt to run migrations. Perhaps something like skipMigrations/--skip-migrations. This also assumes that migrations are handled manually by the user.

Motivating example

The issue is that if one bundle's the worker with webpack, the ./sql directory is not included in the final bundle and the following code causes Graphile worker to fail at startup: https://github.com/graphile/worker/blob/b6bb9db7c9e6851598db470f1681a96ea15353ee/src/migrate.ts#L72

There are various ways around this, for instance just including an empty ./sql directory where the worker expects it to be. All would be workarounds though.

Worth noting that adding this feature would somewhat be synonymous with adding official support for do-it-yourself migrations.

Supporting development

I:

  • [x] am interested in building this feature myself
  • [x] am interested in collaborating on building this feature
  • [x] am willing to help testing this feature before it's released
  • [x] am willing to write a test-driven test suite for this feature (before it exists)
  • [ ] am a Graphile sponsor ❤️
  • [ ] have an active support or consultancy contract with Graphile

joelmukuthu avatar Sep 16 '20 15:09 joelmukuthu

@joelmukuthu If your concern is webpack bundling; it might be better to solve this by having the migrations converted to JS and imported (rather than reading from fs). This is what we do for PostGraphile via the make-assets script:

https://github.com/graphile/postgraphile/blob/v4/scripts/make-assets

I'd be happy to have a build script that took this approach - generating a single TypeScript file of the migrations (an array of objects, each object containing the filename and the SQL: export const migrations: Array<{name: string, sql: string}> = [...]) before running tsc. Would that work for you; and are you still interested in building the solution?

benjie avatar Sep 17 '20 07:09 benjie

Sounds good, and I agree that that's a better approach when the use-case is bundling with webpack. On the other hand, if one runs migrations manually, I'd imagine one would rather not have the migration files in the production bundle. So they could be two different features.

Which feature do you think makes more sense for the project? I can make some time to work on either or both :)

joelmukuthu avatar Sep 17 '20 08:09 joelmukuthu

Honestly I doubt a server bundle cares about a few kb. Let’s aim for webpack compatibility. Hell... we may as well webpack the code we upload to npm so it’s just one file, then everyone wins. We’ll still need a separate CLI entrypoint.

benjie avatar Sep 18 '20 07:09 benjie

For everyone else struggling to bundle the server with webpack here are the loaders needed to get all Critical dependencies fixed, all sql files copied and all __dirname path adjusted in the code:

{
  test: [
    // We do not use pg-native so we don't have to load the native part of the pg package
    require.resolve('pg/lib/native'),
    // Disables require calls made by graphile-worker
    require.resolve('import-fresh'),
    require.resolve('graphile-worker/dist/getTasks')
  ],
  loader: 'null-loader'
},
// Relocate all sql files needed for the migrations
{
  test: require.resolve('graphile-worker/dist/migrate'),
  loader: '@zeit/webpack-asset-relocator-loader',
  options: {
    outputAssetBase: 'graphile-worker'
  }
}

mlegenhausen avatar Oct 07 '20 14:10 mlegenhausen