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

Convert component generator to TS #632

Merged
merged 12 commits into from
Jun 3, 2020
Merged

Convert component generator to TS #632

merged 12 commits into from
Jun 3, 2020

Conversation

kimadeline
Copy link
Contributor

@kimadeline kimadeline commented May 31, 2020

For #523

✅ What it does

  • Generator:
    • component/templates/ files converted to TS
    • templates are transpiled to JS if we're generating JS files
    • generated files pass yarn rw lint
  • component/__tests__ and fixtures converted to TS
    • Added some test cases as well
  • CLI:
    • support for --typescript and --javascript args for yarn rw g component (also supports --ts and --js to save myself some keystrokes, but I can remove/update that if needed)
    • generator refuses to run if files exist in the same language
    • generator re-runs if --force is used
    • default is --javascript=true at the moment

🔮 What needs to be verified

  • The generated code doesn't pass yarn rw test, but that might be because I used a previous version of Redwood to create my test app, and the templates have changed since then:

image

🚧 What it doesn't do (yet)

  • The generator hasn't reached full awareness yet: the default is JS, and it doesn't do environment detection (of tsconfig.json for example). Do you know where this code (probably a common helper) should go, or if anybody already got started on it? Add getLanguage() helper for project typescript | javascript default and settings #633
  • If existing files are a different language target (e.g. in the case of a hybrid-project), the generator should refuse to run regardless of using the --force option;
  • If TS files are generated, the jsconfig.json file in testapp/web should have the "jsx": "preserve" option, which should probably added when creating the app. Thoughts? set jsx preserve in web/jsconfig create-redwood-app#61
  • New CI tests are so-so, I need to make the tests use packages/cli/src/lib/__tests__/fixtures/prettier.config.js but the paths are set up as such as they are pointing to /path/to/project/prettier.config.js, and, well, there's nothing there 😞
    • A better solution would probably be to mock transformTSToJS in component.test.ts, and test transformTSToJS's output separately in lib/index.test.ts or something.

👏 Credits

This PR is "inspired" by #515 (transformTSToJS is basically a copy-🍝 with added flair) and #557 (took your index.d.ts and added dirty ambient module declarations).

type: 'boolean',
default: false,
describe: 'Generate TypeScript files',
alias: 'ts',
Copy link

Choose a reason for hiding this comment

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

👍 alias!

Copy link
Contributor

Choose a reason for hiding this comment

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

💯this should be default setup across generators for sure!

retainLines: true,
}).code

return prettify(filename.replace(/\.ts$/, '.js'), result)
Copy link

Choose a reason for hiding this comment

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

Now that we have .tsx, does all the extension magic still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pass the final .js output filename to transformTSToJS here, so teeeechnically the PR doesn't use this replace logic. I can remove it, and it can be re-added whenever we end up needing it. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote to remove. But defer to your decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in master now, so I'll amend it to cover the .tsx case in case other generators end up needing it too.

@peterp
Copy link
Contributor

peterp commented Jun 2, 2020

Hey @kimadeline, thanks for this! I'm going to merge @jmreidy's PR soon and then I'll get to this next!

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Amazing! Thank you so much for this!

@peterp peterp added this to the next release milestone Jun 3, 2020
@peterp
Copy link
Contributor

peterp commented Jun 3, 2020

New CI tests are so-so, I need to make the tests use packages/cli/src/lib/tests/fixtures/prettier.config.js but the paths are set up as such as they are pointing to /path/to/project/prettier.config.js, and, well, there's nothing there 😞

I would love to start a discussion around refactoring the generator code to make it easier to test and to extend, I'll open up an issue with some ideas at tag you!

@thedavidprice
Copy link
Contributor

@kimadeline huge thanks for getting this rolling!

Just to make sure, did you see that @jmreidy is going to be unavailable for awhile -- new baby!! 🎉

I'm working on #633 this morning. It's going to be a very simple isTypescript() returning a boolean, which can be used to set the command default language target.

@peterp Did you see this Q above?

If TS files are generated, the jsconfig.json file in testapp/web should have the "jsx": "preserve" option, which should probably be added when creating the app. Thoughts?

^^ +1 to add to template on my end. I opened redwoodjs/create-redwood-app#61 -- just merge or close as needed.

@kimadeline
Copy link
Contributor Author

Whoa hadn't seen it, thanks for the heads-up @thedavidprice, and congrats to you and your family @jmreidy 🎉

I'll merge master and get my test app straightened out hopefully this evening (more realistically this weekend 😬).

@peterp
Copy link
Contributor

peterp commented Jun 3, 2020

I'll merge master and get my test app straightened out hopefully this evening (more realistically this weekend 😬).

@kimadeline I opened a pull-request that does all that on your fork! :)

…erator-to-ts

Kimadeline convert component generator to ts
@peterp peterp merged commit 9ea29e7 into redwoodjs:master Jun 3, 2020
@kimadeline kimadeline deleted the convert-component-generator-to-ts branch June 3, 2020 17:21
@thedavidprice thedavidprice mentioned this pull request 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.

4 participants