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

feat: create initial TS template #868

Merged
merged 11 commits into from
Oct 8, 2024
Merged

feat: create initial TS template #868

merged 11 commits into from
Oct 8, 2024

Conversation

kabaros
Copy link
Contributor

@kabaros kabaros commented Jul 31, 2024

implements LIBS-594

Implementation Details:

A templates directory containing the init files directories for both the JS and TS templates has been added. Files like package.json, App.module.css, and package.json don't change for either of them hence they are contained in the common directory.

The TS template contains all the same init files as the JS template with a few more files for TS configuration, i.e:

  1. tsconfig.json
  2. declaration files: maintaining 2 separate declaration files under the types directory, i.e. the global.d.ts and modules.d.ts. (Merging them results in the css modules declaration being invisible).
  3. eslint.config.js
  • This is an initial configuration file that a user can build upon after setting up the project.

  • To test this, a user can add a custom lint command to the project scripts in the package.json, for example: "lint": "eslint ." and run yarn lint. Or just run 'npx eslint .' directly in the current project directory.

  • Alternatively, there is also the option of using the dhis2-cli-style configurations. Start by installing it with yarn add @dhis2/cli-style --dev and then setting up the scripts in your package.json as shown here.
    You could also adjust the eslint.config.js file to add the configurations provided by d2 styles. (To be tested: d2 imported config may not be compatible with the flat config system)

       import config from '@dhis2-cli-style'
       import js from '@eslint/js'
       import tseslint from 'typescript-eslint'
    
       export default [
          config.eslintReact,
          js.configs.recommended,
          ...tseslint.configs.recommended,
          { // other configurations go here }
      ]
    

How to test:

  • Run the init script to create a new TS project. For example, within the app-platform directory, you could run

    cli/bin/d2-app-scripts init test-ts-project --typescript
    
  • Change into your newly created project to test the application. Because of changes made to the start script as well, you could run the start script directly from the app-platform project. For example, following the test-ts-project created above:

    cd test-ts-project 
    ../cli/bin/d2-app-scripts start
    

@kabaros
Copy link
Contributor Author

kabaros commented Jul 31, 2024

@d-rita to test this, I run ./app-platform/cli/bin/d2-app-scripts init test-project --typescript (replace ./app-platform with path to where you have the app-platform project). This will create a new test-project in the current directory.

For yarn start to work in the newly bootstrapped project, there are some changes to start command that need to be incorporated. Since these changes are not published yet, I use yalc to publish from /app-platform/cli

image

then install this version into the bootstrapped project with yalc add @dhis2/[email protected], then yarn start should work

image

maybe @KaiVandivier has a better workflow for testing changes to cli and init/start scripts?

@kabaros kabaros force-pushed the typescript-template branch from e970f5a to 4464cef Compare July 31, 2024 17:59
@KaiVandivier
Copy link
Contributor

KaiVandivier commented Aug 5, 2024

Good progress here! I'm excited to see this in action 🙂

For the testing workflow, I haven't actually had to test out any work that has involved changes to both init and a start or build command, so I don't actually have a workflow planned for this case -- yours looks good though! I can share some other options for your consideration too:

  1. You can init a new app from outside the repo like you described above: ./app-platform/cli/bin/d2-app-scripts init test-project --typescript
    1. (It probably helps to install the latest alpha in the new app here too)
    2. Then you can run the CLI scripts locally using relative paths in the same way: ../app-platform/cli/bin/d2-app-scripts start (can update the scripts in package.json) -- this may save running yalc publish/push, though I'm not too familiar with yalc
  2. You can also init an app the same way, then run a script to copy app-platform/cli into test-project's node_modules/@dhis2/cli-app-scripts (and delete .d2/shell/node_modules/.cache, if needed) -- then regular yarn d2-app-scripts start should work
  3. I often end up using the example apps to test CLI changes -- to test a newly initialized app, it would also be possible to run yarn d2-app-scripts init test-project in the monorepo root (which will use the local d2-app-scripts), then add the newly initialized app to the repo workspaces. When the @dhis2/cli-app-scripts version in the new app matches the local package version in the workspace, it should use the local one

Here are some other, mostly subjective tips:

  1. For the init files, my instinct would be to keep them all in one init dir instead of having a dir for JS and one for TS, since they’re already copied one-by-one instead of as a batch (e.g. a new app only gets one of the two d2.config.js files, depending on if it's a lib or an app; and libs don't get App.module.css or App.test.jsx) — this would make the pathing a little easier, and avoids duplicating files like App.modules.css, package.json, and README.md across two init directories
  2. For the TS/JS files (like entrypoint.jsx), since there are no types in the TS versions yet, it might be fine to keep one init file around (e.g. just the .jsx version, but could be either), then give it a TS extension when copying it into a new project if the TS flag is set. If you add types to the .ts(x) files, then leave them as their own files of course 👍
  3. Don’t forget docs 😊

I'll add a few other things to specific files 👍

@@ -0,0 +1,68 @@
const defaultsApp = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this file is needed -- these are more JS code to import as defaults, rather than boilerplate to be copied into a new app

(I actually meant to add a comment about this when separating these objects from the boilerplate files, but forgot to add it in the last PR, so it's in a more recent one: d2ConfigDefaults.js)

cwd = cwd || process.cwd()
cwd = path.join(cwd, name)
fs.mkdirpSync(cwd)
const paths = makePaths(cwd)
const paths = makePaths(cwd, { typeScript })
Copy link
Contributor

Choose a reason for hiding this comment

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

[Random brainstorming that I think is out of scope for this PR:] It would be cool if passing the ts flag here wasn't necessary, but I think that would need a refactoring of the paths to use something like like globs, which may end up being more complex

// "@typescript-eslint/eslint-plugin"
// "@typescript-eslint/parser"

// ToDO: install any other TS dependencies needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this include types/react[-dom]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even @types/jest

Screenshot 2024-08-05 at 5 31 30 PM

@@ -242,6 +280,12 @@ const command = {
type: 'boolean',
default: false,
},
typeScript: {
Copy link
Contributor

@KaiVandivier KaiVandivier Aug 5, 2024

Choose a reason for hiding this comment

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

Suuuper subjective and equally minor since the alias is set up, and maybe I’m crazy, but I’ll share in case it’s in the back of your mind too: --typeScript feels weird to me as a flag or as a variable name 😅 I think of it as typescript or ts — it seems like you even used --typescript too when you wrote out CLI command script for your workflow

cwd: paths.base,
})

// Todo: check why d2 start is renaming TS files to JS
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still happening? I haven't noticed it

Note that some files from the app shell will be copied in to .d2/shell/, which are called App.jsx and App.test.jsx, which may be confusing:

Screenshot 2024-08-05 at 5 32 38 PM

@d-rita d-rita force-pushed the typescript-template branch from 4464cef to 53b048e Compare August 22, 2024 05:26
- add custom declaration files for React and CSS modules
- install relevant type definitions: react, jest, etc
- add initial eslint config file
- JS files reside in init dir
- TS files reside init-typescript dir
- Common dir contains files used by both templates, i.e. README.md, package.json and App.module.css files to reduce duplication
- update eslintignore file with new file paths
@d-rita d-rita force-pushed the typescript-template branch from 53b048e to 4220ede Compare September 3, 2024 01:16
@d-rita d-rita marked this pull request as ready for review September 11, 2024 09:39
@d-rita d-rita requested a review from a team September 11, 2024 09:39
@kabaros kabaros merged commit 2795f79 into alpha Oct 8, 2024
5 of 6 checks passed
@kabaros kabaros deleted the typescript-template branch October 8, 2024 11:13
dhis2-bot added a commit that referenced this pull request Oct 8, 2024
# [12.0.0-alpha.16](v12.0.0-alpha.15...v12.0.0-alpha.16) (2024-10-08)

### Features

* create initial TS template ([#868](#868)) ([2795f79](2795f79))
* migrate snap files too ([#878](#878)) ([521f483](521f483))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 12.0.0-alpha.16 🎉

The release is available on:

Your semantic-release bot 📦🚀

dhis2-bot added a commit that referenced this pull request Dec 13, 2024
# [12.0.0](v11.7.5...v12.0.0) (2024-12-13)

### Bug Fixes

* **deps:** upgrade app-runtime and ui ([#895](#895)) ([8ed0ec3](8ed0ec3))
* **deps:** upgrade react to 18 in example apps ([#900](#900)) ([7fd16d7](7fd16d7))
* **deps:** use npm v6 before publishing ([01ad502](01ad502))
* **init:** update bootstrap script branch ([#896](#896)) ([33c261a](33c261a))
* **plugin:** clean up resize observer and handle sonarqube warnings ([#898](#898)) ([f113dd5](f113dd5))
* alerts from plugins [LIBS-695] ([#881](#881)) ([21be0d2](21be0d2))
* allow serving files from cwd node_modules ([0233949](0233949))
* base url env & refactor [LIBS-635] ([#872](#872)) ([7f19259](7f19259))
* bump ui library ([#882](#882)) ([1ae9569](1ae9569))
* clear only required build dirs ([#894](#894)) ([179305f](179305f))
* env refactor for Vite wrap-up [LIBS-690] ([#889](#889)) ([84da4e6](84da4e6))
* injectPrecacheManifest warning logging ([a0d266e](a0d266e))
* normalize to .js extensions when compiling libraries ([#893](#893)) ([58b33a8](58b33a8))
* **service-worker:** handle undefined config vars in injectPrecacheManifest ([a90a4e0](a90a4e0))
* correct app shell paths ([#883](#883)) ([a1af71c](a1af71c))
* handle jsx in js support [LIBS-633] ([#871](#871)) ([595a35d](595a35d))
* increase precache max file size to 3 MB ([b20ed22](b20ed22))
* remove custom eslint from TS template ([71cef4b](71cef4b))
* update deps ([1e7ce93](1e7ce93))
* update workbox deps to avoid deprecation warnings ([9a81c4a](9a81c4a))
* use strings for 'boolean' env vars ([eaf5e66](eaf5e66))

### Features

* create initial TS template ([#868](#868)) ([2795f79](2795f79))
* enable HMR for .js files ([5f4683c](5f4683c))
* handle plugins with Vite [LIBS-610] ([#863](#863)) ([ca5be0d](ca5be0d))
* jsx migration script ([#869](#869)) ([7764f49](7764f49))
* migrate snap files too ([#878](#878)) ([521f483](521f483))
* replace CRA with Vite [LIBS-598] ([#847](#847)) ([3dd0e59](3dd0e59))
* upgrade react to v18 ([#876](#876)) ([77ecf10](77ecf10))
* **init:** set direction: 'auto' and import locales for new apps [LIBS-645] ([#867](#867)) ([4eda4a9](4eda4a9))

### BREAKING CHANGES

* require react version 18

* fix: pin react version to 18

* test: update test and test libraries for react 18
* Supported Node versions are 18.x or 20+

* ci: upgrade Node version

* fix: always add PWA_ENABLED to app env for better static analysis

* chore: pause precache manifest injection

* fix: building SW without CRA

* chore: comment update

* fix: group moment locales in their own dir

* refactor: clean up precache injection

* fix: locale utils and handling moment in jest

* fix: compile correctly after merging changes

* chore: comment in compile.js

* chore: some clean-up

* chore: comments

* fix: use port 3000 for the dev server

* fix: improve moment locale chunk naming

* chore: remove CRA

* fix: use mjs build of Vite

* fix: bump cli-style for CRA fix

* feat: use interactive dev server output from Vite

* fix: make dev server port configurable

* chore: remove old index.html

* fix: env tweaks
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 12.0.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants