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

enable typescript services and sdl #515

Merged
merged 25 commits into from
Jun 3, 2020

Conversation

jmreidy
Copy link

@jmreidy jmreidy commented May 5, 2020

What

Enables the generation of sdl and service files in TypeScript. Uses the --typescript boolean flag passed to generate service and generate sdl. Default remains javascript, but we'll reduce templates to only include TS (and generate the JS via transpiling the TS output).

Implements portions of #523.

Testing

Added unit tests to cover TS service and SDL files

Implementation Plan

  • Add typescript flag and template to sdl
  • Add typescript flag and template to service
  • Update sdl tests
  • Update service tests
  • Add Prisma typescript types to generated TS services
  • Ensure sdl and service generation work with TS via scaffold command
  • Update sdl to use typescript, and generate JS via transpilation
  • Update service to use typescript, and generate JS via transpilation
  • Add javascript flag to service, default to true
  • Add javascript flag to sdl, default to true
  • Test all combinations of destroy and generate
  • Enable destroy with TS

@cannikin
Copy link
Member

cannikin commented May 5, 2020

Tested locally with redwood-tools. Is there a way of testing via unit tests?

Check out the __tests__ directory under each generator, there are tests that a file is created at the correct path and that the generated content matches a fixture.

@jmreidy
Copy link
Author

jmreidy commented May 5, 2020

@cannikin thanks, I'll mark this as draft until i add tests

@jmreidy jmreidy changed the title enable typescript services and sdl [wip] enable typescript services and sdl May 5, 2020
@peterp
Copy link
Contributor

peterp commented May 6, 2020

@jmreidy another approach that we've been thinking of is to make the templates typescript by default, but to transpile them to JavaScript and to format them through prettier once they're generated.

The reason for this is because I think having to maintain two separate files for the same template might be a bit cumbersome.

@jmreidy
Copy link
Author

jmreidy commented May 6, 2020

@peterp makes sense. How would a consumer enable JS instead of TS? Pass a --typsescript false flag?

@peterp
Copy link
Contributor

peterp commented May 6, 2020

@jmreidy Yeah, I think that makes sense.

Once we fully support TypeScript I think we'll store the type of project in redwood.toml and then default to that in the generator, but allow a flag to override it.

@thedavidprice
Copy link
Contributor

Excited about this. Thanks @jmreidy

How would a consumer enable JS instead of TS? Pass a --typsescript false flag?

Just to be clear, for the near-term any generate command should default to JS and require using an option for TS. So in this case, the source will be TS and will by default be transpiled/formatted to JS when generated.

I'm suggesting we maintain JS default across the entire project until JS is no longer the default across the entire project. Does this make sense and do you agree @peterp?

@thedavidprice
Copy link
Contributor

Ah, also some potential overlap here with scaffold generation that will need to be handled.

@cannikin If the SDL and Service templates are written as TS files, then the generate scaffold command will also need to handle them appropriately as well, correct?

@cannikin
Copy link
Member

cannikin commented May 6, 2020

That's true, the scaffold generator makes a call over to the sdl and service generators' files() function to get the list of files to create... so yeah it would need to pass along any new flags that are sent to the scaffold generator!

@cannikin
Copy link
Member

cannikin commented May 6, 2020

If I can throw in a flag idea, if we're defaulting to typescript I would think just passing along --javascript would be enough, rather than --typescript=false. I always like "positive" flags to change behavior rather than negative ones, if it makes sense in context.

@peterp
Copy link
Contributor

peterp commented May 6, 2020

I'm suggesting we maintain JS default across the entire project until JS is no longer the default across the entire project. Does this make sense and do you agree @peterp?

💯, agreed.

@jmreidy
Copy link
Author

jmreidy commented May 8, 2020

@cannikin realizing as well that if we generate TS SDL files, and then run scaffold, scaffold would check for the existence of a js file and generate one.

I'm hesitant to add typescript to scaffold, since converting web files to use TS is not nearly as straightforward right now (until changes to Router and Cells land etc).

I'm considering the following:

  • enable a typescript flag for service and sdl generation, as in this PR
  • update scaffold to check for the existence of js OR ts service and SDL files when executing

What do y'all think?

@cannikin
Copy link
Member

cannikin commented May 8, 2020

Hmm, I don't love the idea of only the sdl and services generators supporting TS and not the others...ideally all the generators should offer the same experience. But I guess we have to start somewhere?

What do you think @thedavidprice and @mojombo? How's the developer experience with only those two generators being typescript enabled? Maybe if you try the --typescript flag with any of the other generators you get some console output telling you "Typescript support is coming soon" and we can replace them one at a time?

@jmreidy
Copy link
Author

jmreidy commented May 8, 2020 via email

@thedavidprice
Copy link
Contributor

thedavidprice commented May 8, 2020

Handling existing files

update scaffold to check for the existence of js OR ts service and SDL files when executing

I think this should be true of all the generators, i.e. check for the existence of both TS and JS when executing, and then throw an Error if either exists, regardless of the target being TS or JS. Users can override with --force.

@cannikin do all the generators currently check for existing files before running?

Scaffold TS support

If it's not going to result in a runtime Error, and if we can communicate the behavior clearly, I'm all for users being able to generate TS files for SDL and Services even though Web will be JS. Most likely this would require a pre-run explanation with user confirmation to proceed.

see my comments in the section below explaining --no-javascript usage

So maybe:

  1. command yarn rw g scaffold [model] --no-javascript
  2. CLI output TS support is only available for SDL and Service file types. All Web components will be generated as JS files. Would you like to proceed? y/n

?

Generators Requiring Router Support

We can still make the conversion but only allow/enable target=JS (i.e. always transpile when the command is run) until dependencies like the Router are ready.

Or am I missing something here?

Making TS the default Target but enabling JS target as default (for now)

[edit: this is outdated thinking on my part -- see #523 for a much clearer path to address this]

If our goal is to make TS the eventual default for Redwood, I'm wondering how best to navigate user behavior changes during this transition. So imagining a future example:

  1. user runs yarn create redwood-app [dirName] --javascript --> The default install is TS without the flag. So the user adds the --javascript flag to target JS.
  2. During installation, the redwood.toml project config is set to target=javascript
  3. Whenever a generator command is run, the redwood.toml setting is used as the default
    • a user can override the default with the appropriate --typescript or --javascript option

So in the future, we'll have a default target to work with. But we don't right now.

  • should we add a default now (via Webpack) to set target=javascript, which we can then start accessing across all generator commands as we convert?
  • and then we can have the generators start 1) reading the App default target and 2) allowing it to be overridden with --typescript

I might be overthinking this as we can just add both --javascript and --typescript options now and set --javascript to 'true' as the Yargs default. But then we'll need to come back and add-in the "first check App config target" behavior at a later time.

??

Organizing Effort with a Tracking issue

Seems like we are going to need some organization here across the entire effort of converting generators to TS. I've created a Tracking Issue #523

Please take a look and definitely discuss and suggest edits. Once we have consensus on the plan, I'll see if we can get additional community support from these kind folks as well. (@jmreidy would be great to discuss how/if you and Mohsen might want to help with organizing the effort -- no pressure but let us know if interested.)

@cannikin
Copy link
Member

cannikin commented May 8, 2020

@cannikin do all the generators currently check for existing files before running?

Yup, they'll stop as soon as they run into a file that exists. You'll need to --force to overwrite.

I like the idea of just adding --javascript and --typescript now and defaulting one now and the other later when we have widespread TS support. But no matter what I heartily veto --no-javascript please!

@jmreidy
Copy link
Author

jmreidy commented May 9, 2020

@thedavidprice hats off for fantastic documentation and breakdown in that tracking issue. Thanks so much for the clarifying commentary both there and in this thread.

Two last questions from me:

  • do we want to rewrite the yargs generator files themselves as TS?
  • how should we batch these changes? 523_typescript_templates feature branch?

And yes, happy to help organize the effort and pull in folks from the community, just let me know if you have any specific guidance on how you'd like me to proceed there.

@peterp
Copy link
Contributor

peterp commented May 9, 2020

do we want to rewrite the yargs generator files themselves as TS?

@jmreidy We're going to modify the architecture of the CLI and generators slightly - I'll involve you in those discussions when they happen, but I don't think we'll get much value out of converting those now.

@peterp peterp marked this pull request as draft May 9, 2020 20:30
@peterp peterp force-pushed the master branch 6 times, most recently from 711c520 to 2341368 Compare May 10, 2020 10:07
@guledali
Copy link
Contributor

Maybe it should look more like this?

type-arguments

If you look at blitz they are using the generated types from prisma client.
https://github.com/blitz-js/blitz/tree/canary/packages/generator/templates/query
https://github.com/blitz-js/blitz/tree/canary/packages/generator/templates/mutation

@thedavidprice
Copy link
Contributor

@jmreidy Thanks for your patience here as we catch up with your trailblazing! The core team discussed this yesterday. I've updated #523 to address questions here as well as provide a roadmap for proceeding (including some redundancy of previous decisions).

I'm revising the 523 as we learn more, so please add your comments and questions as needed.

Questions from previous comments in this PR

realizing as well that if we generate TS SDL files, and then run scaffold, scaffold would check for the existence of a js file and generate one.

I'm hesitant to add typescript to scaffold, since converting web files to use TS is not nearly as straightforward right now (until changes to Router and Cells land etc).

^^ I attempt to address the first part of this in #523 tl;dr generators need to be both TS/JS aware for creating, overwriting, and refusing to overwrite in the case of hybrid scenario.

And for the second part, hybrid generators are fine during this transition. E.g. TS SDL and Services with JS web/ components is ok (for now).

  • however, this is unexpected behavior, so a CLI message+explanation should be given with prompt to "accept and continue"

do we want to rewrite the yargs generator files themselves as TS?

^^ no, not at this time

how should we batch these changes? 523_typescript_templates feature branch?

^^ the goal is to transition in steps, meaning each PR will focus on incremental, working changes. This might not always be possible, so we'll adapt as we go. But in this case, it seems right now we can support SDL, Service, and hybrid-Scaffold as an incremental step that does not change existing JS behavior or expectations

@jmreidy
Copy link
Author

jmreidy commented May 30, 2020

Good news! I got it working!

(video)

Bad news - rebasing is a massive pain. Especially after https://github.com/redwoodjs/redwood/pull/611/files, which rewrote all the scaffold templates.

I'll need to keep working through conflicts, if we can hold on making changes to templates in the meantime, I'd appreciate it...

jest.mock('fs')
jest.mock('@babel/core', () => {
Copy link
Author

Choose a reason for hiding this comment

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

babel is unhappy when you mock fs

Copy link
Contributor

Choose a reason for hiding this comment

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

We're thinking of creating a wrapper around fs with a few common operations like write, read, unlink in @redwoodjs/internal that should be slightly more manageable to mock.


expect(Object.keys(files).length).toEqual(2)
})
const itReturnsExactly2Files = (baseArgs) => {
Copy link
Author

Choose a reason for hiding this comment

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

noted in a comment elsewhere - extracted tests into named function to reduce duplication across TS and JS versions

}

const getBaseFile = (file) => file.replace(/\.\w*$/, '')

Copy link
Author

Choose a reason for hiding this comment

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

Instead of adding typescript as a flag to destroy, it felt more ergonomic to me for it to just destroy whatever js/ts variation is found.

@jmreidy
Copy link
Author

jmreidy commented Jun 1, 2020

This should be good to go but will need someone to take it over the finish line - I just had a baby!

@thedavidprice
Copy link
Contributor

@jmreidy Woah! Huge congrats to you and yours!! 🎉😍

We can't thank you enough for all the sustained effort you put into this, Justin. It's created so much momentum and paved the way by example. You're going to be missed. But you've made a huge impact here. And we'll be excited to see you again in the future when/if you're available months from now.

One thing that might help --> could you give @peterp (and maybe me) commit access to jmreidy:convert_api_to_ts? Not necessary if you don't get around to it.

If for some reason we get absolutely stuck, I’ll reach out to you directly apart from GitHub.

Congrats again!

@peterp peterp self-assigned this Jun 3, 2020
@peterp peterp merged commit 409bbf3 into redwoodjs:master Jun 3, 2020
@peterp
Copy link
Contributor

peterp commented Jun 3, 2020

Thanks for this @jmreidy!

@peterp peterp deleted the convert_api_to_ts branch June 3, 2020 08:52
@thedavidprice thedavidprice added this to the next release milestone Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants