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

Add command for Typescript typedefs generation #79

Merged
merged 36 commits into from
Mar 14, 2024
Merged

Add command for Typescript typedefs generation #79

merged 36 commits into from
Mar 14, 2024

Conversation

Edo-San
Copy link
Contributor

@Edo-San Edo-San commented Feb 13, 2024

Pull request type

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Other (please describe):

How to test this PR

I have set up an npm dev script that can be useful to conveniently run the build step and the CLI in a "pseudo-dev mode".

  • Log in
  • Locate a file with a components JSON Schema (or generate one via the pull-components command)
  • Run the generate-typescript-typedefs command providing the components JSON Schema via the --sourceFilePaths flag
  • Inspect the generated Typescript definition file
  • Try out different flags and check the output

Command example

npm run dev generate-typescript-typedefs -- --sourceFilePaths ./components.12345.json

What is the new behavior?

The main purpose of this PR is to add a command to the CLI that would provide our users with first-party support for generating Typescript type definitions based on Storyblok component schemas. As we're dealing with Typescript, it was a good opportunity to open up Typescript adoption in the CLI as well, as it made working with Typescript MUCH easier.

Here's a list of the provided changes:

  • Create a GenerateTypesFromJSONSchemas class responsible for the whole rendering cycle of the Typescript type definitions file.
  • Add support for Typescript files and build process with unbuild - required updating all import types to be ESM
  • Add .vscode settings to prevent automated formatting of existing js files while working on ts files
  • Add a generate-typescript-typedefs CLI command and task file
  • Add Typescript types for the whole task and its internals under the new directory types
  • Add a section to the README file related to the generate-typescript-typedefs command

TBD / Potential issues / Need hints

  • As for now, the generated type definitions file is importing the ISbStoryData type from the same storyblok package, but there are some issues with exporting those types from this package, namely because some of them are also being imported from the storyblok-js-client. I'd assume that any user who is willing to use this new command is going to rely on the storyblok-js-client, at some point or another, so I guess the way to go is to somehow export the types alongside the package.json with @storyblok/js or storyblok-js-client listed as dependencies so that we can be sure we're going to import types that are going to be available from our distributed types. I still have to find out how to do that properly :D
  • Maybe we need automated tests for this (read below)? There are a lot of options/combinations to handle, it's hard to test them all manually. Furthermore, updating CJS to ESM imports caused unit tests to broke, so I need to figure out how to fix them/refactor them.

Potential improvements

As I had the chance to dig further into the CLI repo, I have outlined some potential improvements, for both the generate-typescript-typedefs command and the whole repo:

  • [TS] Create a way to avoid clashes between StoryblokProvidedPropertyTypes and bloks (components) that can be created in Storyblok with the same name. I.e. a user can create a blok named table, and the types will end up having two TableStoryblok types, one for the user's component and one for the Storyblok-provided Table type.
  • [TS] Give the possibility to the user to code the custom type parser in Typescript
  • [TS] Add automated tests (need setup for TS tests) - Do you think this is required? It could also be a good opportunity to set them up with TS and to provide some mocks that could also be useful as internal examples, such as a customFieldTypesParser or a JSONSchemaToTSOptions.
  • [CLI] Code formatting is missing
  • [CLI] Unified error handling (maybe a dedicated class?)
  • [CLI] Typescript support on all the codebase
  • [CLI] There are undocumented commands, like scaffold, that are not documented in the README file. Maybe we could benefit from some cleanup. That command in particular I think is related to the old Rendering Service.

Other information

A new build step should be added to trigger the build script.

This work has been heavily inspired by dohomi's Storyblok generate ts. Thank you for your hard work and support! 🙏

@Edo-San Edo-San requested review from ademarCardoso and a user February 13, 2024 11:46
@Edo-San
Copy link
Contributor Author

Edo-San commented Feb 23, 2024

Update 23 Feb 2024

I refactored all the tests so that they could run with ESM.

@Edo-San Edo-San changed the title [WIP] - Add command for Typescript typedefs generation Add command for Typescript typedefs generation Feb 28, 2024
@Edo-San
Copy link
Contributor Author

Edo-San commented Feb 28, 2024

Update 28 Feb 2024

I bumped the storyblok-js-client version to 6.7.1 because that includes exporting a TS interface that was hindering the build process ATM. Now running npm run build should successfully build the CLI Node executable and it should also export its type definitions.

Copy link
Member

@ademarCardoso ademarCardoso left a comment

Choose a reason for hiding this comment

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

This is HUGE, amazing refactor @Edo-San 🚀

Please take a look in this comment, it's very important.

Just to make a note:

This PR will generate a breaking change, it will be necessary to publish as V4.0.0

@Edo-San
Copy link
Contributor Author

Edo-San commented Mar 6, 2024

This is HUGE, amazing refactor @Edo-San 🚀

Please take a look in this comment, it's very important.

Just to make a note:

This PR will generate a breaking change, it will be necessary to publish as V4.0.0

Thank you @ademarCardoso Appreciate! 🤗

I wanted to ask you about the breaking changes, as I am probably missing something. Aside from the fact that with this new build step, there won't be the possibility to directly import anything from the CLI, as we won't be exposing all the CLI files anymore, do you see any other breaking changes?

UPDATE 06/03/2024

I rebased the PR on the updated master branch and fixed the conflicts related to the merged #81 PR

@ademarCardoso
Copy link
Member

This is HUGE, amazing refactor @Edo-San 🚀
Please take a look in this comment, it's very important.
Just to make a note:
This PR will generate a breaking change, it will be necessary to publish as V4.0.0

Thank you @ademarCardoso Appreciate! 🤗

I wanted to ask you about the breaking changes, as I am probably missing something. Aside from the fact that with this new build step, there won't be the possibility to directly import anything from the CLI, as we won't be exposing all the CLI files anymore, do you see any other breaking changes?

UPDATE 06/03/2024

I rebased the PR on the updated master branch and fixed the conflicts related to the merged #81 PR

Due to the amount of changes in the project, I thought it would be a breaking change, but I talked to other engineers and since the cli itself won't change the user's behavior, in other words, I don't think anything will break, so it's not a breaking change.

@ademarCardoso
Copy link
Member

@Edo-San I don't know if there is a QA on your team, but I would recommend that this PR be reviewed by one, to ensure that everything is working as it should. Carlos is an excellent QA and has experience with the CLI

@carlosericciardi
Copy link

There is an error when the component has numbers at the beginning of its name:
image

command: npm run dev generate-typescript-typedefs -- --sourceFilePaths ./components.139799.json

@Edo-San
Copy link
Contributor Author

Edo-San commented Mar 11, 2024

There is an error when the component has numbers at the beginning of its name: image

command: npm run dev generate-typescript-typedefs -- --sourceFilePaths ./components.139799.json

Pushed a fix for this one!
Basically, Typescript identifiers cannot start with a digit. I added some logic that, after all the transformations, check if the type name starts with a number and, if that's the case, it appends an underscore to the type name.

Thanks again for pointing that out @carlosericciardi 🙏

Copy link

@carlosericciardi carlosericciardi left a comment

Choose a reason for hiding this comment

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

Awesome work!

@Edo-San
Copy link
Contributor Author

Edo-San commented Mar 12, 2024

@ademarCardoso Do you think we can merge it now that @carlosericciardi has reviewed it? You can merge it straight away if you want 🤗

@Edo-San
Copy link
Contributor Author

Edo-San commented Mar 14, 2024

Merging it as we good greenlight from @gustavomelki

UPDATE

Reverted because a build step is missing on the CI

@Edo-San Edo-San merged commit 3123308 into master Mar 14, 2024
1 check passed
@Edo-San Edo-San deleted the WEB-790 branch March 14, 2024 09:12
Copy link

🎉 This PR is included in version 3.28.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants