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

[WIP] TypeScript Spike 🦔 #16713

Merged
merged 2 commits into from May 3, 2023
Merged

[WIP] TypeScript Spike 🦔 #16713

merged 2 commits into from May 3, 2023

Conversation

allouis
Copy link
Contributor

@allouis allouis commented Apr 27, 2023

🍦

🤖 Generated by Copilot at c605838

This pull request refactors the post-revisions package from JavaScript to TypeScript, improving the code quality and type safety. It also updates the imports of the PostRevisions class in the core package and the dev script in the .github folder. Additionally, it removes some redundant code from the sodo-search package.

const log = console.log;
/* eslint-enable no-console */

fs.copyFile('./public/main.css', './umd/main.css', (err) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file doesn't exist since 21b621c

I had to update this because it was messing up the ability to run lerna run build from monorepo root

"build": "tsc --build",
"prepare": "yarn build",
"setup": "yarn build",
"test:unit": "NODE_ENV=testing c8 --src src --all --check-coverage --100 --reporter text --reporter cobertura mocha -r ts-node/register './test/**/*.test.ts'",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ts-node means we don't need to add a compile step for tests - I think this is preferable for now

"test:unit": "NODE_ENV=testing c8 --all --check-coverage --100 --reporter text --reporter cobertura mocha './test/**/*.test.js'",
"dev": "tsc --build --watch --preserveWatchOutput",
"build": "tsc --build",
"prepare": "yarn build",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means that the project will be built after running yarn

// "disableReferencedProjectLoad": true, /* Reduce the number of projects loaded automatically by TypeScript. */

/* Language and Environment */
"target": "es2019", /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK This is correct for Node 14

Copy link
Member

Choose a reason for hiding this comment

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

Given we've dropped Node 14, you can probs bump this now

@@ -33,7 +33,7 @@
"main": "yarn main:monorepo && yarn main:submodules",
"main:monorepo": "git checkout main && git pull ${GHOST_UPSTREAM:-origin} main && yarn",
"main:submodules": "git submodule sync && git submodule update && git submodule foreach \"git checkout main && git pull ${GHOST_UPSTREAM:-origin} main && yarn\"",
"prepare": "husky install .github/hooks"
"prepare": "husky install .github/hooks && lerna run prepare"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works with the post-revision prepare script so that running yarn in the top level will build all necessary packages

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -12.64 ⚠️

Comparison is base (cb30c9b) 84.70% compared to head (8adc500) 72.07%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main   #16713       +/-   ##
===========================================
- Coverage   84.70%   72.07%   -12.64%     
===========================================
  Files         692     1787     +1095     
  Lines       53396   113379    +59983     
  Branches     6832    16921    +10089     
===========================================
+ Hits        45228    81713    +36485     
- Misses       8097    30564    +22467     
- Partials       71     1102     +1031     
Flag Coverage Δ
e2e-tests 84.67% <100.00%> (-0.03%) ⬇️
unit-tests 56.26% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ghost/core/core/server/models/post.js 93.12% <100.00%> (+3.13%) ⬆️
ghost/core/core/server/services/users.js 98.01% <100.00%> (ø)
ghost/post-revisions/src/PostRevisions.ts 100.00% <100.00%> (ø)
ghost/post-revisions/src/index.ts 100.00% <100.00%> (ø)

... and 1289 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@allouis allouis force-pushed the wip-typescript-spike branch 6 times, most recently from 166708f to c492fd2 Compare May 1, 2023 18:35
@@ -231,11 +231,13 @@
"eslint": "8.37.0",
"expect": "29.3.1",
"form-data": "4.0.0",
"ghost-admin": "5.46.0",
Copy link
Member

Choose a reason for hiding this comment

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

Do you reckon we can use a * here? It's gonna be a pain to keep the version in sync (more code in Ghost-Release) and I'd like to clean up the process that means we need this, but not necessarily now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't see why not!

"inquirer": "8.2.5",
"jwks-rsa": "3.0.1",
"mocha": "10.2.0",
"mocha-slow-test-reporter": "0.1.2",
"mock-knex": "TryGhost/mock-knex#d8b93b1c20d4820323477f2c60db016ab3e73192",
"monobundle": "allouis/monobundle",
Copy link
Member

Choose a reason for hiding this comment

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

I can/will move my original one to TryGhost once we resolve the PR there

- Updates the prepare script in the top level to run prepare on packages, so
  that packages can be built when running `yarn`

- Updates the build script in ghost/core to run build on packages, so that
  packages are built before being monobundled

- Updates monobundle to be a dependency and use the new TryGhost repo, which
  includes some minor fixes and improvements, such as supporting devDeps

- Updates the GitHub workflows to run the build command in the top level
  directory rather than ghost/core so that other packages are built, too.
This is an initial start to using TypeScript in our non-core Ghost packages.

- Adds a prepare script to build the project after installing deps
- Adds an initial tsconfig.json which is compatible with our node env
- Migrates all of the code to TypeScript, including tests
- Updates tests to use ts-node so that we don't need to compile the tests
- ts-node is installed at the top level because the env is weird with lerna and
  doesn't work otherwise
- Updates the yarn dev script to build the project with the --all and --revisions flag
@allouis allouis merged commit b9565bc into main May 3, 2023
12 of 13 checks passed
@allouis allouis deleted the wip-typescript-spike branch May 3, 2023 18:32
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.

None yet

2 participants