blitz icon indicating copy to clipboard operation
blitz copied to clipboard

[legacy-framework] Subtemplate management

Open Roesh opened this issue 3 years ago • 46 comments

Closes: blitz-js/blitz#2038

What are the changes and their implications?

Continuing PR blitz-js/blitz#2134 , using @Maastrich 's regex functions in the generator.

This PR improves the generator scaffolding process, allowing files like mutations, pages and forms to include fields specified by the user, rather than have a default "name" field.

Sub tasks

  • [x] Support the following replacement cases: field_name, Field_name, Field_Name
  • [x] Update form to use this format: {/* template: <__FieldComponent__ name="__fieldName__" label="__Field_Name__" placeholder="__Field_Name__" /> */}
  • [x] Abstract out getTemplateValues into the base Generator class, and allow subclasses to add or override the default values.
  • [x] support // template: ... in addition to /* template: ... */ Note: Supported, but only either one can exist in one file. Works like the previous logic we had, but we may want to support multiple template comments per file actually, so once that is decided, we can check this box. (EDIT: Leaving this as it works presently)
  • [x] Don't hardcode a display component in the template comment, rather use __FieldComponent__, then provide a map in blitz.config.js or in templates/config
    • [x] Research if the new template key should be placed under the experimental key, or if it can be one of the top level keys
    • [x] Determine how expensive await loadConfigAtRuntime() is. If it is expensive, create a proxy to hold the typeToComponentMap so that multiple fields can use the config fetched during the first call to get the config object.
  • [x] Determine if the logic that sets the BLITZ_APP_DIR variable is correct. The following files check if the variable is already set, and if not, sets it to the current directory using "."
    • [x] packages\generator\src\generators\template-builders\builder.ts (New abstract builder class)
    • [x] packages\generator\src\generators\page-generator.ts (Existing page generator class)
  • [x] Ensure that subsequent runs of blitz generate use the existing file as a template if it exists.

Feature Checklist

  • [ ] Integration test added (see test docs if needed)
    • [x] form-generator test added
  • [ ] Documentation added/updated (submit PR to blitzjs.com repo) Docs PR is https://github.com/blitz-js/blitzjs.com/pull/577

Roesh avatar Aug 27 '21 23:08 Roesh

For the display component aspect of this, I was wondering if we could strongly type the field mappings in blitz.config.ts

import {LabeledTextField} from "app/core/components/LabeledTextField"

fieldComponents: {
    string: {
        name: LabeledTextField,
        importStatement: `import {LabeledTextField} from "app/core/components/LabeledTextField"`
    }
} 

Based on my understanding of how JS works and some searching, this does not seem possible (In c#, there is a nameof operator, but JS does not seem to have such a concept.)

Shall we leave these untyped and in the documentation inform users to ensure that the value corresponding to the type should have the correct name and path?

Im adding the path key because I haven't found an ideal way to infer the path to an object based on its reference. I'll search for this some more, but if there is a known way to do this, please let me know.

So we could do:

fieldComponents: {
    string: {
        name: LabeledTextField,
        importStatement: `import {LabeledTextField} from "app/core/components/LabeledTextField"`
    }
} 

EDIT: After discussing with Brandon, we decided the importStatement prop isn't needed because we include the imports by default in our generators

Roesh avatar Aug 30 '21 14:08 Roesh

Progress update 09/03/2021:

Working on support // template: ... in addition to /* template: ... */

Currently in generator.ts, we have a single fieldTemplateRegExp.

My current solution is create a separate regex for the // template: Previous regex: /{?\/\* template: (.*) \*\/}?/ New regex: /\/\/ template: (.*)/

The alternative is to use a single regex but have multiple capture (/{?\/\* template: (.*) \*\/}?|\/\/ template: (.*))/. I may go that path because we later use the fieldTemplateRegExp. (The issue I'll need to solve is how to use the correct capture group in the right place - $2 captures within {/* */}, $3 captures within // template: )

Will continue to update once every couple days, feel free to pitch in with suggestions!

Roesh avatar Sep 04 '21 00:09 Roesh

Today's progress update:

Working on making the base Generator class define the getTemplateValues by unmarking it as an abstract function.

Next steps were to identify the template values that are common among all generators so that we can return an object that contains the maximum number of overlapping values instead of reusing code. Not all these values have been created already, so I will have to go through each existing generator, see what it will need, and once that is done, update the base Generator's getTemplateValues to return them.

One potential issue is that the app-generator does not require any model or model fields, so it won't be using any of the values that the others do. Another issue to solve is updating the GeneratorOptions base interface in Generator.ts to contain any properties that are needed by the base Generator to generate those values. This is because child generators, like FormGenerator extend the base GeneratorOptions with the properties like modelName and add those to the object returned by their getTemplateValues method

Roesh avatar Sep 11 '21 03:09 Roesh

Progress update 09/12:

When creating the object returned by getTemplateValues we rely on the options being passed in containing properties such as extraArgs and so on. Will need to add these to the base Generator's options so that it can access these and build the options

Roesh avatar Sep 12 '21 20:09 Roesh

Progress update 09/13:

Abstraction of getTemplateValues is complete. The base Generator class has a default implementation that children can override. Initially, these problems were present

  • One generator (model generator) does not implement any getTemplateValues
  • Another generator (app generator) have a completely different set of values

All other generators did output several common fields (like parentModels, modelId, etc.) This could have been solved by making the base generator class return the most common values while app generator and model generator would override conditionally

The issue here is that in order to generate the above options, we need the "extra args" and other models provided into the options argument passed in. This would require adding all of those options into the base generator's options interface.

I personally did not like that the base generator would have a ton of parameters that would have to be set as optional (because not all generators (like the app generator) would provide "modelName", "modelNames" etc.), while their children when extending the base options, would require them to be passed in.

My solution was to create a separate Builder class that would be responsible for generating these field template values. The base generator class would now "have" a public templateValuesBuilder, and the base class' getTemplateValues() will call this builder's getTemplateValues. Now, each child class would specify the builder it wanted to use. Because everything other than the app generator and model generator share ~90% of the values, there are three concrete values builders, which inherit from an abstract template builder:

  1. AppValuesBuilder: Used when generating templateValues during new app generation
  2. NullBuilder: Used by default in the base Generator class, and also used by ModelGenerator which does not override the base
  3. FieldValuesBuilder: Used by all other Generators, including FormGenerator, MutationsGenerator, etc.

The base abstract builder class also holds methods like getId, getParam, etc. which were originally duplicated across generators themselves.


  1. Getting component from blitz.config.ts map:

Using const {loadConfigAtRuntime} = await import("next/dist/server/config-shared") to get the values stored in blitz.config.ts. Running into the same Error: Internal Blitz Error: process.env.BLITZ_APP_DIR is not set that Andreas is here: https://github.com/blitz-js/blitz/pull/2699

I believe the pages-generator's postWrite function is also having the same issue so will have to resolve it in all spots.

Roesh avatar Sep 13 '21 21:09 Roesh

Progress update 09/14/2020:

Didn't have as much time to work on this today, but I haven't found the best solution for populating the process.env.BLITZ_APP_DIR variable before calling the loadConfigAtRuntime

Doing process.env.BLITZ_APP_DIR = "." works, but I might need to ask Brandon if there is a better way this variable needs to be initialized so that we can cover all use cases?

Roesh avatar Sep 15 '21 02:09 Roesh

Progress update 09/15/2021:

Added the template to component map support in biltz.config.ts. This required updating the NextConfig type in the nextjs\packages\next\server\config-shared.ts file.

The issue is whether this should be nested under the experimental property or if it can stay at the top level. The other issue is understanding how expensive await loadConfigAtRuntime() is. If it is expensive, we may need to create a proxy to hold the typeToComponentMap so that multiple fields can use the config fetched during the first call to get the config object.

Roesh avatar Sep 16 '21 04:09 Roesh

Need to rewrite the form-generator test. It runs locally but fails during CI, I'm assuming because the builder that gets the configuration uses await loadConfigAtRuntime() which doesn't work in CI, causing config.template to be undefined.

Roesh avatar Sep 21 '21 23:09 Roesh

OK, the fix for issue https://github.com/blitz-js/legacy-framework/issues/302 should resolve the tasks regarding BLITZ_APP_DIR for this PR. Once that is closed, I can check them off here.

09/23: We pushed out a quick fix for that issue, but we did not address

  1. What the best place to set the BLITZ_APP_DIR was
  2. What it should be set to
  3. Integration test to ensure generator commands work and generate the files / output needed

I will ask Brandon about 1) and 2) if it isn't clear after a few hours examining how BLITZ_APP_DIR and options.destinationRoot are used across the generators and more broadly, across the codebase

09/26: Brandon informed me that the BLITZ_APP_DIR variables function was used to find the blitz.config file at runtime. His fix is done at the "cli command" level (see PR https://github.com/blitz-js/blitz/pull/2736/files)

I will be using this same fix

Roesh avatar Sep 23 '21 01:09 Roesh

@Roesh you are a gem for working on this!!

flybayer avatar Sep 30 '21 23:09 flybayer

Thanks haha! Just noticed one of the CI failing tests was one I wrote, let me fix that. I noticed the failing Windows one but I think that test is being worked on elsewhere?

I'm sure there's a few corrections to be made here and there, things like naming conventions for the files etc. Maastrich's regex code is doing most of the heavy lifting in terms of adding in multiple fields so most of that should be OK. If there are any additional issues, please let me know!

Roesh avatar Oct 01 '21 00:10 Roesh

@Roesh ok awesome!! I will review next week.

@beerose can you take first pass through reviewing and testing this? Then I can do a more final review.

flybayer avatar Oct 02 '21 20:10 flybayer

I noticed that after generating a new model, the template line remains in the form component. Is it expected?

Yes, that way after can run blitz generate a second time to add more fields to a model

flybayer avatar Oct 05 '21 15:10 flybayer

Update 10/06/2021:

Pulled in the changes from the uuid support PR.

Finished supporting uuid in the mutations template, need to do the same for the prisma generator (currently that PR allows non-id fields to be uuids, but does not affect the primary key) EDIT: that PR also works for the primary key - it didn't work for me because I was trying to overwrite an existing model. Question is whether we want to support that. Generating a new model with id:uuid works

Also, I'm not sure what the best way is to get the parentModelIdType for scaffolding. I might need to parse the current prisma schema and get the appropriate id that way. The current model's id type is determined by an optional id:type argument the user passes in, along with the other fields.

The field values builder uses this extra id argument to update a modelIdZodType key added to the returned values object.

  • [x] Obtain the id type of an existing parent model so that the create zod schema can correctly specify it
  • [ ] Check if we should support updating a prisma schema if a new cli command updates the types of the primary key or a different field
  • [ ] Research if the Fallbackable<T> type is better to use for the component and zod maps, since we're creating fallback maps if the blitz.config.ts/js doesn't have the keys we need. In addition, should we display an error message if the fallbacks were used?

Almost done with getting the parent models id type, but having issues with the generation of the new.tsx file

IDENTICAL app\pages\testmodels\new.tsx
✕ SyntaxError: Illegal return statement

Roesh avatar Oct 06 '21 18:10 Roesh

Great! All of that makes sense. I actually didn't notice the blitz g subsequent run issue because I didn't modify a generated file so I assumed we were using the generated file already, but since that isn't the case I'll need to update the generator logic to check for a generated file in the location it will be generated to, then use that file and only add or update fields.

For the maps, I think that will work. I was actually wondering if we need to have an extra Prisma type field. Based on the support uuid PR, it might make sense to update the prisma's Field Type to be dictated by this map as well?

Also, I want to confirm that the types in the map (string, boolean, etc.) will need to be specified in the documentation. The docs currently have json as an extra type, but doesn't specify bigint, so I assume we'll want to update the types map with a json key, and add bigint to the docs?

Roesh avatar Oct 15 '21 19:10 Roesh

Progress update 10/17/2021:

Consolidated the types used across resource generators.

TODO:

  • [ ] Noticed that the model generator isn't adding a relation field if we use the --parent flag. Looks like this is because it isn't part of the extra args which are what are used when those are generated. Need to create a separate issue to track this
  • [ ] Looks like this is also not fully addressed for fields that reference another model, looks like we need to call prisma format after the model is generated: https://github.com/blitz-js/blitz/issues/2864 (Could either have someone work on this separately or I'll get to this after the tasks I've worked on)

Roesh avatar Oct 17 '21 12:10 Roesh

@Roesh

Config map:

  • Yes we can add prismaType if we need that. So essentially the keys inside fieldTypeMap are what we accept as arguments to blitz generate.
  • I'm now thinking codegen would be a better top level config key than template(s) since codegen is more general.

And yes, need to update docs for field types.

flybayer avatar Oct 19 '21 16:10 flybayer

Progress update 10/21/2021:

  1. Tried to access prisma's functionality that performs prisma format, but wasn't very successful. I was rummaging through what the prisma node_modules folders exposed, and none of them have the Format function I am looking for.
  • [x] Research to see if there is a way to make a command line call from a generator function
  1. Updated the template config type to be the new version we wanted, updated the new apps defaults with Brandon's suggestions.
  • [x] Need to move it below the commented out webpack config
  • [ ] Update generators to work with the new structures
  • [ ] Update the prisma generator to get the prisma type from the codegen map

Roesh avatar Oct 21 '21 20:10 Roesh

@Roesh here's how you can run prisma format: https://github.com/blitz-js/blitz/blob/canary/packages/cli/src/commands/new.ts#L320-L322

But do we already do that?

flybayer avatar Oct 21 '21 21:10 flybayer

Good question. I've got to check, but I assumed we didn't because in https://github.com/blitz-js/blitz/issues/2864, someone is running into an issue which would have been solved if prisma format was run after model generation.

OK, so it looks like the post-write command is running prisma migrate dev but right now were not formatting before that. Maybe in the past prisma migrate dev first ran prisma format?

I added functionality to do prisma format before prompting the user for the migration and this causes the relationship to be set correctly.

Ok, there's a weird issue where Typescript isn't recognizing spawn.sync, going to look into that. The cross-spawn index.d.ts that's being imported seems fine, and other places in the app are able to do the same thing:

const result = spawn.sync(blitzBin, ["--version"], {
      cwd: exampleProject,
    })

Going to troubleshoot this for a little bit but mighth have to ultimately resort to ts-ignore if I can't figure it our (the functionality itself works)

EDIT: switching to blitz-js/blitz#2864 to discuss this, will post any updates there

Roesh avatar Oct 21 '21 21:10 Roesh

Going to work on supporting multiple blitz generates updating existing files this week, I estimate it should be done sometime between Wednesday or Friday.

So, running into a strange issue, I check to ensure a file already exists using
const destinationExists = fs.existsSync(final_destinationPath)

but I get an error when trying to copy that file

✕ Error generating __modelIdParam__\edit.tsx
✕ AssertionError [ERR_ASSERTION]: Trying to copy from a source that does not exist: D:\QuickDocs\blitz\examples\auth\app\pages\test-models\[testModelId]\edit.tsx

I logged what final_destinationPath has and its blitz:generator destionation D:\QuickDocs\blitz\examples\auth\app\pages\test-models\[testModelId]\edit.tsx +0ms

meaning the file does exist but for some reason copy.js is throwing that error. Need to look at node_modules\mem-fs-editor\lib\actions\copy.js to see why I think its because of the "[" and "]" characters, but will need to confirm

While I'm debugging that, thought I'd check to see if the other parts work. Deleted the files that have "[" and "]" and found that the logic I've used works, but the overwrite prompt is being bypassed. Need to fix that also. EDIT: Unable to reproduce this, going back to try solve the file existence problem.

In copy.js, in the exports.copy function

// Sanity checks: Makes sure we copy at least one file.
  assert(options.ignoreNoMatch || files.length > 0, 'Trying to copy from a source that does not exist: ' + from);

is the exact location that's failing. files.length is 0, and the value of from at this point is D:\QuickDocs\blitz\examples\auth\app\pages\test-models\[testModelId]\edit.tsx. Logging those, I see that both D:\QuickDocs\blitz\examples\auth\app\pages\test-models\index.tsx and D:\QuickDocs\blitz\examples\auth\app\pages\test-models\new.tsx were found files.length = 1 for these files and the appropriate source file is used as well. The form component is also copied with the correct updates when the failing files are deleted.

Need to look higher up in that same function to determine how it knows if a file exists.

Roesh avatar Oct 23 '21 13:10 Roesh

Progress update 10/25/2021:

Continuing to investigate the copy issue, looks like its coming from var diskFiles = globby.sync(fromGlob, globOptions);

fromGlob is set on all paths, but diskFiles is only set for paths that dont end with "[" and "]"

node_modules\globby\index.js > module.exports.sync (Line 138)

10/26/2021: https://github.com/mrmlnc/fast-glob#advanced-syntax Regexp character classes ([1-5]). Escapes characters (\) — matching special characters ($^*+?()[]) as literals. https://github.com/mrmlnc/fast-glob#escapepathpattern

So I think the way this was solved before by the code was by not adding special characters before the file copy, but after it was done. We currently need to be able to copy from a file that has special characters though, so I am using the escapepath() function provided by fast-glob to make sure the paths we're passing in are OK.

Roesh avatar Oct 25 '21 23:10 Roesh

Update 10/27/2021:

Need to resolve the Lint issue caused by us not explicitly specifying fast-glob in dependencies or peer dependencies

Update 10/29/2021:

The dependency of fast-glob is reached like so: packages\generator\package.json specifies "mem-fs-editor": "8.0.0", node_modules\mem-fs-editor\package.json specifies "globby": "^11.0.1" node_modules\globby\package.json specifies "fast-glob": "^3.1.1"

Neither globby nor mem-fs-editor expose the escapePath function, only fast-glob does.

@flybayer and @beerose, would it make more sense to specify fast-glob as a dependency to packages\generator\package.json? I'm not sure how wise this is, even if we write tests in case there are new patterns that are escaped?

The alternative would be to copy what's in the fast-glob escape function, but this again would run into the same problem above

Roesh avatar Oct 27 '21 19:10 Roesh

@Roesh you are a champ!! I'm so thankful you are working on this.

Adding fast-glob as a dep sounds goods.

flybayer avatar Oct 30 '21 21:10 flybayer

Ayy, thank you :)))

Cool, I'll do that

Roesh avatar Oct 31 '21 14:10 Roesh

So I think that's all of the functionality that we needed so far. I'm going to start writing tests, including integration ones where we generate files and examine the output to make sure everything's OK. I might uncover things during that process, but hope to have some tests done by end of week

And fix the Lint issues. I'm having trouble running lint and other testing locally, husky doesn't seem to be working so I've had to --no-verify my commits, but I might try to fix that too

Roesh avatar Nov 02 '21 01:11 Roesh

@Roesh awesome!

flybayer avatar Nov 02 '21 21:11 flybayer

OK, so almost done, based on our conversation about integration testing, it looks like we'll want to wait for the CLI cypress integration tester to be complete, so all that's left is the accompanying documentation for this

The other thing to note are improvements that can be made to the incremental generation of fields. Right now, we're defaulting to using an existing file and adding fields specified in the new command, but there could be additional generation "modes" which we can consider supporting:

  1. "From scratch" mode - will wipe the existing resources and replace them with the resources generated by the generate command, useful to quickly discard existing work
  2. "Template override" mode - will use the existing resources if they exist, but will delete all field specific lines (things like the form fields and zod inputs, basically anything generated by the output of the 'Builder' classes. Useful if someone wants to keep their pages etc., but just want to both delete and/or update resources
  3. "Add" mode, which is what we do now, which uses an existing file if present and adds any new fields to it, and if the file doesnt exist, generates it

Roesh avatar Nov 14 '21 03:11 Roesh

This is amazing work, thank you for this.

imcodingideas avatar Nov 14 '21 18:11 imcodingideas

Thanks guys! I've added the fields argument to the docs here: https://blitzjs-com-92luv5hui-blitzjs.vercel.app/docs/cli-generate

@flybayer We have an example in I'm wondering if it would make sense to update the generated files to have specific fields like description:string and priority?

We currently have this in the docs:

blitz generate all project will generate the following files:
XXX
For the above example, you can view the generated project index page at localhost:3000/projects

Im considering updating that to

blitz generate all project description:string priority:number will generate the following files:
XXX
For the above example, you can view the generated project index page at localhost:3000/projects

And the blitz new command will generate a project with those fields?

Roesh avatar Nov 16 '21 21:11 Roesh

@Roesh ugh, so sorry for delay in testing this out again. I've offloaded a lot of my other blitz work now. So I can follow up here more quickly now.

  • yes you can update that doc you mentioned
  • [ ] looks like you have a failing unit test: https://github.com/blitz-js/blitz/runs/4245479178?check_suite_focus=true
  • [ ] Adding a new field after first generation fails
    • cd to examples/auth/
    • Run blitz g all test name:string
    • Run blitz g all test description:string
    • Fails with file doesn't exist, but the file does exist
    ✕ Error generating __modelIdParam__/edit.tsx
    ✕ AssertionError [ERR_ASSERTION]: Trying to copy from a source that does not exist: 
    /Users/b/c/blitz/examples/auth/app/pages/tests/\[testId\]/edit.tsx
    
  • [x] Previous comment still pending. I tried adding that and looks like __inputType__ is not used as a template variable. My intention was that every key inside the codegen config (inputType, zodType, component, etc) would be available as a template variable. I guess this could be added later, but I think generating a number field type is common enough that it would be good to have in initial release.
  • [x] I also just realized there's an opportunity for standardization because we use zodType in both config and template, but we use component in config and FieldComponent in template. So maybe we should use FieldComponent in the config too?
  • [x] Prisma type is not being read from blitz.config correctly
    • I added a number type to blitz.config fieldTypeMap:
        number: {
          component: "LabeledTextField",
          inputType: "number",
          zodType: "number",
          prismaType: "Int",
        },
    
    • Then blitz g model apple price:number results in prisma Number instead of Int
    $ blitz g model sauce price:number
    ✔ Model 'Sauce' created in schema.prisma:
    >
    > model Sauce {
    >   id        Int      @id @default(autoincrement())
    >   createdAt DateTime @default(now())
    >   updatedAt DateTime @updatedAt
    >   price     Number
    > }
    >
    
  • [ ] Update mutation is still left with an extra line in the zod schema. (generated with blitz g all test 'name:string?' price:number) Not a big deal, but would be nice to be fixed
    const UpdateProduct = z.object({
      id: z.number(),
    
      name: z.string(),
      price: z.number(),
      // template: __fieldName__: z.__zodTypeName__(),
    })
    

flybayer avatar Nov 18 '21 00:11 flybayer

Thats alright! Totally understandable,

I'll get to a few of these at once, and once you verify they're complete, you can check them off.

inputType issue: I made a function that gets the input type just for this, but totally forgot to add the key to the getFieldTemplateValues function. Also updated __ModelName__Form.tsx so it now generates this:

<Form<S> {...props}>
      <LabeledTextField name="name" label="Name" placeholder="Name" type="text" />
      {/* template: <__FieldComponent__ name="__fieldName__" label="__Field_Name__" placeholder="__Field_Name__"  type="__inputType__" /> */}
    </Form>

Ah just saw this: My intention was that every key inside the codegen config (inputType, zodType, component, etc) would be available as a template variable. Yeah I think I'll need to update the way getTemplateValues is built so that it iterates over the keys in that config file and uses those keys to build instead. So I can add that as a TODO item. EDIT: I updated the logic to add keys based on whats in the config. It basically checks if the fieldType has a map under it, then adds those keys to it. There may be a more efficient way of doing it compared to the loop Im using now (destructure into values maybe?)

Failing unit test: Updated it to take into account the inputType variable that was added. Going to wait until github actions re-runs, haven't been able to resolve the running of tests locally.

11/19/2021:

Prisma type is not being read from blitz.config correctly:

I left a comment that I ended up stashing and forgetting about, oops!

In packages/generator/src/prisma/field.ts, line 62 needs to be modified

let fieldType = capitalize(_fieldType) //TODO: Need to make the generic function in tempalte buiolders a utility accessiable here.
    // Check if it would make sense to expose that to users as well?
    // Also in the case of a relationship, need to use the raw model name, cant capitalize it.

11/20/2021:

Ok, so I'm going to make Prisma get values from the codegen key in blitz config, but I'd like to confirm a change to the existing Field.parse function:

// 'name:type?[]:attribute' => Field
  static parse(input: string, schema?: ast.Schema): Field[]

I might need to make that function async because we'll need to await a call to loadConfig - const {loadConfigAtRuntime} = await import("next/dist/server/config-shared")

Its only used in one spot though: packages\generator\src\generators\model-generator.ts line 42

let field = (extraArgs.length === 1 && extraArgs[0].includes(" ")
      ? extraArgs[0].split(" ")
      : extraArgs
    ).flatMap((input) => Field.parse(input, schema))

Roesh avatar Nov 18 '21 22:11 Roesh

11/21/2021:

Ok, so I'm having issues with getting the config from the field.ts file, unsure why it isn't picking the config up, hmm

blitz:config Loading config from  D:\QuickDocs\blitz\examples\auth\.blitz.config.compiled.js +2s
  blitz:config Did not find custom config file +241ms

image

Now that I think about it more, I need to make sure the other location I use this same call to const {loadConfigAtRuntime} = await import("next/dist/server/config-shared") is working too. I think I do a try catch block and pass in a fallback component map if it fails so I need to make sure this isn't happening within the try block

11/23

OK, so I did some testing today and its magically working? I am not sure why, but looks like starting, then closing and restarting works? Going to push that change blitz g model test field1:number

blitz:config Loading config from D:\QuickDocs\blitz\examples\auth.blitz.config.compiled.js +405ms

✔ Model 'Test' created in schema.prisma:

model Test { id Int @id @default(autoincrement()) createdAt DateTime @default(now()) updatedAt DateTime @updatedAt field1 Int }

I think it might make sense to add a debug type for codegen, so that we can debug things like what map was found, and what was used to generate a particular template

Roesh avatar Nov 21 '21 17:11 Roesh

Adding a new field after first generation fails

Hm, this is a weird one, but after some debugging I think I know what happened. Basically, I don't think the escape characters function is working to fix the special characters issue, and when I worked on the fix, I was looking at the Form.tsx output which was working, but I was ignoring the Edit.tsx and other special character files because I wasn't escaping the line that checked whether to use an existing file or not. (It was basically outputting "IDENTICAL" instead of error, file not found) Before committing, I probably looked at that line and caught that and updated it, but didn't test it, so it looks like this is still TODO

Roesh avatar Nov 22 '21 16:11 Roesh

Title: Blitzjs progress update 11/30/2021 TODO: After some consideration Brandon felt that codegen was going to be easier for upgrading users if it werent present, so we're going to remove that from the blitz.config.ts file and have it defaulted in code.

Next steps:

  • [x] remove from generated code"
  • [x] Make sure everything works like it did before"
  • [x] New issue - make sure uuid type works and specifies the uuid decorator" (Aleksandra made sure the previous UUID PR continued to work in our new generator

Roesh avatar Dec 01 '21 01:12 Roesh

Based on @Vandivier 's opened request, I realized that there is some additional default handling code that is needed. This is for two cases so far:

  1. unspecifed fieldType blitz g all model field doesn't pull a default component. Should we default in string in this case?
<Form<S> {...props}>
      <__component__ name="field" label="Field" placeholder="Field" type="__inputType__" />
      {/* template: <__component__ name="__fieldName__" label="__Field_Name__" placeholder="__Field_Name__"  type="__inputType__" /> */}
    </Form>

Logic for belongsTo must be specified, must intercept this "special name" before its processed as a template field. Right now this happens: blitz g all model belongsTo:user

<Form<S> {...props}>
      <__component__
        name="belongsTo"
        label="Belongs To"
        placeholder="Belongs To"
        type="__inputType__"
      />
      {/* template: <__component__ name="__fieldName__" label="__Field_Name__" placeholder="__Field_Name__"  type="__inputType__" /> */}
    </Form>
  • [x] Handle case when there are no field template values present
  • [ ] Handle case for belongsTo field
  • [x] Fix spacing issue in Form template. I fixed the issue that caused a space to be set in the mutations templates by modifying the regex to capture between newlines on the "//" template comments, might need to do the same for the {/* */} ones as well because now they are adding a space between generates

Roesh avatar Dec 12 '21 02:12 Roesh

We're loading codegen a few times — I'm wondering if we can load it once somewhere top-level and pass it down as we do for other commands with loadConfigProduction. E.g. we could pass it to generators that need it in generate.ts file.

beerose avatar Dec 14 '21 14:12 beerose

We're loading codegen a few times — I'm wondering if we can load it once somewhere top-level and pass it down as we do for other commands with loadConfigProduction. E.g. we could pass it to generators that need it in generate.ts file.

Yeah, I initally was caching it in the builder, but I moved that logic into get-codegen, and I didn't know a good spot to add a cache variable since it wasn't a class. I also don't know how this is done best in Node, should we create a separate class to hold this or can we create a const config variable?

Roesh avatar Dec 14 '21 15:12 Roesh

So I was doing some testing on the belongs to field to make sure it was similar to the one specified with the --parent flag, I don't see the model being generated with a User[] field, but if I use the belongsTo key, I do see those fields.

The field.ts file generates the schema values, so I'll take a look at that once I'm done with syncing up the generator behavior for the two

https://blitzjs.com/docs/cli-generate#options

Ok, so I broke my node_modules again. donig a global search while yarn dev is running always messes things up lol. I was able to update the parent zod type to use the new codegen utility, but will have to test it once the repo is re-installed.

Roesh avatar Dec 14 '21 23:12 Roesh

So I was doing some testing on the belongs to field to make sure it was similar to the one specified with the --parent flag, I don't see the model being generated with a User[] field, but if I use the belongsTo key, I do see those fields.

The field.ts file generates the schema values, so I'll take a look at that once I'm done with syncing up the generator behavior for the two

https://blitzjs.com/docs/cli-generate#options

Ok, so I broke my node_modules again. donig a global search while yarn dev is running always messes things up lol. I was able to update the parent zod type to use the new codegen utility, but will have to test it once the repo is re-installed.

Let me know if you need support wrapping this up. This looks fun. I know there is the belongsTo field case and the todo from @beerose

imcodingideas avatar Dec 22 '21 21:12 imcodingideas

Let me know if you need support wrapping this up. This looks fun. I know there is the belongsTo field case and the todo from @beerose

Thank you @imcodingideas! I'll fix the conflicts shortly. And if you'd want to test it briefly, we'd very much appreciate that!

I also think that the belongTo thing can be covered later (unless it's a regression).

beerose avatar Dec 23 '21 11:12 beerose

Yeah feel free to jump in! I have less time these days so am not be able to contribute as much as I used to :(((

Roesh avatar Dec 23 '21 15:12 Roesh

Any updates on this? Will this be released in short terms? Or else, is there a prisma scaffold generator that anyone could advise. Kind regards

Jarrodsz avatar Dec 24 '21 14:12 Jarrodsz

@Jarrodsz yeah we're trying to ship it very soon.

flybayer avatar Dec 24 '21 15:12 flybayer

Ok, so I should have some free time now. I'm making a virtual machine and installing nvm as well so hopefully between the stability offered by nvm and the ability to take machine snapshots my npm installation should be stable.

I can handle the belongsTo flag in a couple days time I think

Ok, I spoke too soon. I'm having an issue within the virtual machine too. Going to troubleshoot that. It seems to be an issue with libsodium.

For anyone else on Windows, I fixed that by installing windows-build-tools (npm install -g windows-build-tools). Also needed to install python separately because node-gyp wasn't detecting it in that PATH by itself

Ok, running into another error with the ncc_amp_optimizer package. yarn dev runs, but doing blitz -v inside a folder doesn't work, blitz isn't recognized.

12/29 update: Moved the codegen key into the right spot since that doesn't require running the app to test

Roesh avatar Dec 28 '21 20:12 Roesh

Aleksandra helped setting up a stable dev environment by recommending Volta and the following package versions:

"volta": {
    "node": "14.18.1",
    "yarn": "1.18.0",
    "npm": "6.14.15"
  },

I began working on the belongsTo:ModelName feature. I was processing special args in the field-values-builder, but a lot what I'm trying to do is done in packages\cli\src\commands\generate.ts, around line 248:

const generators = generatorMap[args.type]
      for (const GeneratorClass of generators) {
        const generator = new GeneratorClass({
...
        rawParentModelName: flags.parent,
        parentModel: modelName(flags.parent),
        parentModels: modelNames(flags.parent),
        ParentModel: ModelName(flags.parent),
        ParentModels: ModelNames(flags.parent),

I copied those into the field values generator for now, but we might want to make it a separate utility down the line

The prisma schema gets generated fine but the create model which is supposed to have the parent model id as a requirement doesn't work. The template file uses if (process.env.parentModel) { to render the parentModelId field conditionally. I added process.env.parentModel = typeName to field values builder but its still not being set, need to troubleshoot that next

01/19/2020: Ok, I think I found out the reason for that issue. In packages\generator\src\generator.ts line 224 we do templatedFile = this.replaceConditionals(inputStr, templateValues, prettierOptions || {}) This is where the conditional rendering is performed using the process.env.parent variable

Only after that is done, in line 241, do we perform template values replacement. EDIT: I was looking at the wrong function on line 241. The call to modify process.env.parent happens in getTemplateValues(), which is actually called before the process() function, at const templateValues = await this.getTemplateValues() so it should be working fine

~~My next steps are to see if I can create a utility to scan the arguments list and tack on parent to process.env before the call to this.replaceConditionals~~

One thing to note that even if this is fixed, there are additional implications for repeated code generation using generated files. Generated files won't have conditional jsx rendering anymore E.g:

  • Someone generates resources
  • Runs another generate with --parent flag or belongsTo:modelname
  • Parent fields won't appear because the existing generated file doesn't have the if(process.env.xxx){ <conditional component />} block

Roesh avatar Jan 09 '22 21:01 Roesh