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

Proof of Concept: [UIAPPS-177] add cra-template packages for starter-kit #127

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ericrallen
Copy link
Contributor

@ericrallen ericrallen commented Mar 31, 2022

Motivation

To provide a proof of concept example of converting the generation of the starter-kit example app to a create-react-app Custom Template.

In this particular example we're generating two cra-template-* packages, one for TypeScript and one for JavaScript.

Notes

  • When running create-react-app inside of the monorepo, the root-level yarn.lock file is not found and the template's dependencies are installed via npm. This happens with our custom templates and with the default cra-template provided by create-react-app. You can see this issue by running yarn create react-app examples/test-create-react-app and seeing the Installing template dependencies using npm... message and the generated package-lock.json file in the examples/test-create-react-app directory.
  • This approach requires two separate packages with a lot of duplication, which could be reduced by forking react-scripts and adding some logic to the init script to handle toggling between TS/JS, but then we have to maintain and update the react-scripts fork and that's very similar to the sort of thing we're trying to avoid by exploring Custom Templates and offloading the challenges of keeping track of versions and updates to create-react-app.

Open Questions

  • Given the standalone nature of the example apps, should we consider removing them from the Workspace's configuration so that they can manage their own versions of dependencies and not potentially pollute the Workspace?

Changes

Adds two new packages:

  • @datadog/cra-template-apps-starter-kit
  • @datadog/cra-template-apps-typescript-starter-kit

Allows users to scaffold starter-kit apps via invocation of create-react-app and passing the --template argument:

npm init react-app starter-kit --template @datadog/apps-starter-kit
npx create-react-app starter-kit --template @datadog/apps-starter-kit
yarn create react-app starter-kit --template @datadog/apps-starter-kit
npm init react-app starter-kit --template @datadog/apps-typescript-starter-kit
npx create-react-app starter-kit --template @datadog/apps-typescript-starter-kit
yarn create react-app starter-kit --template @datadog/apps-typescript-starter-kit

Testing

yarn create react-app starter-kit-new --template file:path/to/apps/packages/cra-template-apps-starter-kit
yarn create react-app starter-kit-ts --template file:path/to/apps/packages/cra-template-apps-typescript-starter-kit

Note: In the paths above you will need to replace path/to/apps with the relative path to the apps repository from your current working directory.

Releases

Choose one:

  • No release is necessary.
    If you're only updating examples/documentation, this is likely what you want.
  • All packages that need a release have a changeset.

@ericrallen ericrallen added enhancement New feature or request question Further information is requested labels Mar 31, 2022
@ericrallen ericrallen changed the title [UIAPPS-177] [UIAPPS-156] add cra-template packages for starter-kit Proof of Concept: [UIAPPS-177] add cra-template packages for starter-kit Mar 31, 2022
@ericrallen ericrallen force-pushed the feature/create-react-app-template branch 4 times, most recently from 8b74cf2 to 144a30d Compare April 5, 2022 16:59
Eric allen added 3 commits April 5, 2022 15:53
@ericrallen ericrallen force-pushed the feature/create-react-app-template branch from 144a30d to fbae708 Compare April 5, 2022 19:53
Copy link
Collaborator

@joneshf-dd joneshf-dd 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 really cool! Thanks for throwing it together.

I just gave it a code review. Haven't tried the actual steps. I'll try to run through that Tomorrow.

Given the standalone nature of the example apps, should we consider removing them from the Workspace's configuration so that they can manage their own versions of dependencies and not potentially pollute the Workspace?

I get what you're saying about the examples causing a bunch of issues. There are a few reasons they're workspaces.

One reason is so we can actually make sure the versions stay up to date with changes in the repo. E.g. if we change an API in the SDK, we want the examples to also reflect that change. What was happening before is that the examples were getting pretty out of date with what was going on in the SDK.

Another reason they're workspaces is so that they actually get run in CI. Although we don't don't have any tests on these examples, many of them are at least written in TypeScript so we get static analysis that things still typecheck/compile when in CI.

All that said, this isn't the only way to do things. If we can figure out a way to make sure the examples stay up to date, dependencies are always the same as what's in the repo, we run in CI (so we get type checking, linting, formatting, etc.), etc., then I'm personally all for it.

Another thing to be aware of is that we're not really locked into the tooling we've got. We're likely going to want to continue to use yarn, but there's nothing saying we can't use something on top of yarn that manages a monorepo better than it can. I've floated the idea of looking into other tooling (like nx) to make a lot of what we deal with easier. We just haven't focused on that.


I think there's two additional things worth being aware of: we're likely going to remove the starter kit as it is in exchange for a more up-to-date "kitchen sink" example, we're floating the distinction between examples and actual templates.

For the starter-kit in particular, it has sort of grown out of a very old example and has some patterns that aren't really what we want to push forward. We're hoping to just remove it altogether once we have a better sense of the patterns we want people to emulate.

For the templates, we're finishing up the first batch that we're hoping to display more prominently soon. There's still things to figure out here, but we might not even necessarily build on top of CRA at all. It's a thing that works well in isolation, but it creates a host of problems the way we use it in this repo (as you pointed out). So while the end goal is external DX, we also want to keep an eye on our internal DX.

I'm kind of rambling at this point, so I'll just say if it's cool with you, I think we should hold off on merging this until we've finished with the current iteration of templates. That should help inform a bit more about what we want and how we'll get there.

Comment on lines +1509 to +1520
"@datadog/ui-extensions-react@^0.31.0":
version "0.31.0"
resolved "https://registry.yarnpkg.com/@datadog/ui-extensions-react/-/ui-extensions-react-0.31.0.tgz#319b86c842a21598b744d79fc484511a447fd6de"
integrity sha512-+S68mtPevAWPVLCX2RIR5+9jW/pXXmY7mh3r2EKrKizEBa9D8rWdBeZpkI95I2+Q93cSzFj+sQGYnqLkk0L4mg==

"@datadog/ui-extensions-sdk@^0.31.0":
version "0.31.0"
resolved "https://registry.yarnpkg.com/@datadog/ui-extensions-sdk/-/ui-extensions-sdk-0.31.0.tgz#057c5f0d5ea8a96fa43ecf7d4bc0904b445c2dd5"
integrity sha512-iBo6QhbPHSe3V2iSEx0N42ORUb+aLZFarAq0//375y3UqO5Bhbbru8QBIP1WTA2WO0Lvsr+BabYG6DgAiwGAVg==
dependencies:
"@datadog/framepost" "^0.3.0"

Copy link
Collaborator

Choose a reason for hiding this comment

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

🔨 warning: ‏I think something got out of sync here. At the moment, we pretty much never want to depend on the npm registry for a version of a package that we're developing in repo. The reason is so we always have the latest up to date code in all of the examples. I'm not entirely sure why this happened, but we should for sure address this before merging.

Comment on lines +1456 to +1458
"@datadog/cra-template-apps-starter-kit@file:/Users/eric.allen/Development/apps/packages/cra-template-apps-starter-kit":
version "1.0.0"

Copy link
Collaborator

Choose a reason for hiding this comment

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

🔨 warning: ‏Just adding a note that we'll want to figure this out similar to how we deal with other packages internal to the repo before merging.

Comment on lines +24 to +25

**Note**: This template currently uses v17 of React due to the `peerDependency` that `"@datadog/ui-extensions-react"` has on `17.0.2`; once the relevant `@types/` packages for React and ReactDOM are updated and we can move to v18 across the board, we will.
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏I think we can drop this now that we've updated. Is that right?

Suggested change
**Note**: This template currently uses v17 of React due to the `peerDependency` that `"@datadog/ui-extensions-react"` has on `17.0.2`; once the relevant `@types/` packages for React and ReactDOM are updated and we can move to v18 across the board, we will.

Comment on lines +24 to +25

**Note**: This template currently uses v17 of React due to the `peerDependency` that `"@datadog/ui-extensions-react"` has on `17.0.2`; once the relevant `@types/` packages for React and ReactDOM are updated and we can move to v18 across the board, we will.
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏I think we can drop this now that we've updated. Is that right?

Suggested change
**Note**: This template currently uses v17 of React due to the `peerDependency` that `"@datadog/ui-extensions-react"` has on `17.0.2`; once the relevant `@types/` packages for React and ReactDOM are updated and we can move to v18 across the board, we will.

Comment on lines +4 to +5
"@datadog/ui-extensions-react": "0.31.0",
"@datadog/ui-extensions-sdk": "0.31.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏Can we think through how we might keep these versions in sync with the actual versions in the respective packages? Does CRA have some tooling or a pattern for that?

The way it currently works for the other workspaces is that the version gets bumped whenever we make a release. This works because changesets sees one of these @datadog/ui-extensions-* packages version changes and goes and updates all of the dependencies each workspace. That doesn't need to be the only way we do things, it's just how we've done it so far.

The only other place in the repo that's not always in sync is the packages/create-app/src/template/package.json file. Since packages/create-app/src/template isn't a workspace, it doesn't get the auto-update from changesets. Only reason that is the way it is is neglect: we missed that when adding the template feature. So in that regard, adding the CRA templates is no different than what we have now. But, if we could it'd be nice to figure out a way to make it automatically update.

Barring an automatic update, we could throw a script together that checked these versions matched the expected version and run it as part of yarn test. We do that with other things we can't easily automate away. That way at least we get a failure, even if a human has to get involved.

In any case, more than happy to spit ball some ideas you might have. And this isn't blocking, so don't worry if there isn't really a good story here. Just wanted to bring it up in case there was something.

@@ -4,3 +4,4 @@
**/dist
CHANGELOG.md
docs
packages/*/template/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏What's the motivation behind ignoring this stuff? I'm neither for nor against at this point. Just wanting to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants