Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for using SDL codegen for GraphQL types #8417

Merged
merged 8 commits into from
Jun 8, 2023

Conversation

orta
Copy link
Contributor

@orta orta commented May 25, 2023

Adds a redwood config setting which replaces the graphql-codegen for the API with sdl-codegen

Currently untested/validated because Nx freezes on my computer

Now tested and "works on my computer"

@orta orta force-pushed the use_sdl_codegen branch from 779f48c to 246deae Compare May 25, 2023 11:09
babel.config.js Outdated
@@ -34,7 +34,7 @@ module.exports = {
// List of supported proposals: https://github.com/zloirock/core-js/blob/master/docs/2019-03-19-core-js-3-babel-and-a-look-into-the-future.md#ecmascript-proposals
proposals: true,
},
exclude: ['es.error.cause'],
exclude: ['es.error.cause', 'proposal-dynamic-import'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this module is the first one in Redwood which is ESM only (I ported it from Deno) then it was the first case in the codebase where ESM/CJS interact.

This means it was the first time, from the looks of the rest of the codebase, that import("thing") was used. As we target versions of node which all have support for that, I have dropped the babel backporting for the feature so it can be used natively.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to illustrate this change for others, here's some example source code from the CLI:

export const handler = async (options) => {
  const { handler } = await import('./buildHandler')
  return handler(options)
}

And here's the output dist with our current Babel config (Babel transforms await import to an async-like require):

const handler = async options => {
  const {
    handler
  } = await _promise.default.resolve().then(() => (0, _interopRequireWildcard2.default)(require('./buildHandler')));
  return handler(options);
};

And here's the output dist with this change (the code stays the same; Babel changes the spacing a bit but that doesn't matter):

const handler = async options => {
  const {
    handler
  } = await import('./buildHandler');
  return handler(options);
};

if (config.experimental.useSDLCodeGenForGraphQLTypes) {
const paths = getPaths()
const sdlCodegen = await import('@sdl-codegen/node')
const dtsFiles = sdlCodegen.runFullCodegen('redwood', { paths })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the work happens in the sdl-codegen side, there's a good argument that this is flawed (because what if Redwood changes the shape of this paths object) but I copied the types over into the lib.

I think it's reasonable that at some point I chop this down to just the types from Path which are used, and then you aren't accidentally constrained on the shape of the object for anything but the API side.

yarn.lock Outdated
chevrotain: ^10.4.2
checksum: 155795a245d885d6cd3edac43a3eb57c8ba5c178d71b7595e278c3f7879f78511b9796d3b13e37c228cfdba9621715a2af450611b68aa4d58739fbe129e8200d
languageName: node
linkType: hard
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use the dmmf to grab this info, but it doesn't include the comments! So, I can't re-use that approach.

@jtoar jtoar added the release:feature This PR introduces a new feature label May 25, 2023
@jtoar
Copy link
Contributor

jtoar commented May 29, 2023

Quick update on CI. At least for the tutorial e2e project, the problem seems to be the lack of file extensions: https://github.com/redwoodjs/redwood/actions/runs/5079837564/jobs/9126084663?pr=8417#step:7:29

Taking the yarn rw dev command as an example, here's the dist:

const handler = async options => {
  const {
    handler
  } = await import('./devHandler');
  return handler(options);
};

The reason for the error (linked above) is that there's no .js on './devHandler'. I know ESM is stricter than CJS about relative imports and requires that they have file extensions, but since the CLI is still a CJS module, I thought it'd be ok.

We can proceed in a few ways:

  • add file extensions to the dynamic imports of relative paths in the CLI (I went with this for now)
  • change await import of relative files to require
    • I think the change here wouldn't matter since they get transpiled to require statements anyway, but I'd have to double check

It looks like we may also need to set NODE_OPTIONS="--experimental-vm-modules" when running tests.

babel.config.js Outdated
exclude: ['es.error.cause'],
exclude: [
'es.error.cause',
process.env.NODE_ENV !== 'test' && 'proposal-dynamic-import',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this conditional, Jest wants us to pass --experimental-vm-modules to node. I tried that and most of our test suites actually worked save for the CLI and codemods. But I figured it may be easier to just disable this config for testing since we only really have one ES module we're importing in the entire framework

@jtoar jtoar added the fixture-ok Override the test project fixture check label May 29, 2023
@jtoar
Copy link
Contributor

jtoar commented May 29, 2023

Thanks @orta! Hope you don't mind that I pushed a few commits to get CI going and fix merge conflicts

@jtoar
Copy link
Contributor

jtoar commented May 29, 2023

Looks like the smoke test is failing because I didn't fix runScriptFunction in the CLI which is used for yarn rw exec and prerender:

export async function runScriptFunction({

@orta
Copy link
Contributor Author

orta commented May 30, 2023

No worries, good luck, wonder how I missed that other await import in my search

@Tobbe
Copy link
Member

Tobbe commented May 31, 2023

Alternative solution for getting rid of proposal-dynamic-import: #8456
(Ended up landing in the same solution after all...)

Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @orta! Let's get this one in

@jtoar jtoar merged commit 9ba397b into redwoodjs:main Jun 8, 2023
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 8, 2023
@orta
Copy link
Contributor Author

orta commented Jun 9, 2023

Ace, thanks, now I can port my app to use it full yimr

jtoar added a commit that referenced this pull request Jun 9, 2023
* Initial support for using SDL codegen for GraphQL types

* Get it working

* update snapshot

* try adding file extensions to relative paths

* disable exclude proposal for unit tests

* lint fix

---------

Co-authored-by: Dominic Saadi <[email protected]>
@jtoar jtoar modified the milestones: next-release, v5.4.0 Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants