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

Rename types in preparation for TypeDoc #16

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

lolmaus
Copy link
Contributor

@lolmaus lolmaus commented Aug 15, 2023

⚠️ This PR contains commits from #14 and #15. The diff will clear up once those are merged.

This PR is in preparation for TypeDoc documentation.

This PR contains two changes:

  • Renames the root callbacks and properties to callbackCreateProject. This makes them much more obvious, and also removes the ambiguity with root from Project where it's a string.
    It also defines a type for the callback, as it's used in many places throughout the code.
    And renames ProjectMutator type to CallbackMutateProject for consistency.
  • Splits the State type into ScenariosStateRoot and ScenariosStateDerived. This lets me document them separately. The Scenraios prefix scopes the type to its relevant class.

@lolmaus lolmaus changed the title Typedoc rename Rename types in preparation for TypeDoc Aug 17, 2023
type: 'derived';
parent: Scenarios;
variants: Record<string, ProjectMutator[]>;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we discussed on the office hours, this type is entirely private and self-contained so it should go back to being not exported.

If we add documentation to it, the most useful documentation would be on its individual fields. The object itself is really just an help to typescript so we know which fields are available in which states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ef4 Done!

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

This looks good to me, it feels like the change of keys in some of the objects here are all entirely internal so it's not a breaking change.

I'm approving but I'm not 100% sure I should merge without someone who likes knows typescript having a quick look 👍

*
* @returns A {@link Project} instance, optionally wrapped into a promise.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

shouldn't this comment block be "connected" to the following code for it to be picked up? or is it smart enough to know that it refers to the next line of code that isn't whitespace?

Copy link

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

TS changes look good to me 💯

@ef4 ef4 merged commit 786b02b into embroider-build:main Oct 10, 2023
@mansona mansona added the documentation Improvements or additions to documentation label Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants