Skip to content

feat: migrate project to typescript#40

Merged
nicomiguelino merged 17 commits intoScreenly:masterfrom
hereje:feat/migrate-to-typescript
Mar 21, 2025
Merged

feat: migrate project to typescript#40
nicomiguelino merged 17 commits intoScreenly:masterfrom
hereje:feat/migrate-to-typescript

Conversation

@hereje
Copy link
Copy Markdown
Contributor

@hereje hereje commented Mar 18, 2025

Description

Please include a summary of the changes and which issue is fixed. Please also include relevant motivation and context.

Migrate js files to typescript

The following steps were applied:

  • convert js files to ts

  • replace jest with vitest

  • update clean and build scripts

  • update visual testing

  • ignore dist/ directory from tracking on .gitignore

  • Fixes [CHORE] Migrate to TypeScript #32

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

  • Test A (e.g. unit tests, integration tests, etc.)

Checklist

  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation (where applicable).
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Screenshots

If applicable, add screenshots to help explain your changes.

hereje added 3 commits March 18, 2025 21:40
- set type as module on package.json
- add typescript configuration file `tsconfig.json`
- convert *.js files to *.ts and from cjs to esm
- replace jest with vitest and fix broken tests
due to the migration process
- remove unused jest dependencies
- fix broken visual test
- add dist/ directory to clean script
- run clean script before building dist/ on
build script
- set type as commonjs on package.json
@nicomiguelino
Copy link
Copy Markdown
Collaborator

@hereje, thanks! Let me take some time to review your changes, do an end-to-end testing, and help you fix failing checks.

Copy link
Copy Markdown
Collaborator

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

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

Here are my initial comments just to unblock things:

Comment 1

You still need to create an index.js file at the project root directory and should contain the following:

module.exports = require('./dist').default;

Please see this example for reference.

Comment 2

This is temporary. I'll create a pull request to have the code linting on a concurrent/separate job so that it would not block any processes. Please comment the following lines in .github/workflows/test.yml:

- name: Lint
run: |
npm run lint
npm run format -- --check

If you can, just create a new job called .github/workflows/lint.yml and put the snippet above in this new file.

hereje added 2 commits March 19, 2025 00:19
disable linting for now so that it does not block
any process.

this is a temporary fix, it should be resolved
later
Copy link
Copy Markdown
Collaborator

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

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

@hereje

  • I just merged #41 so that the linter workflow is separate from the test workflow. Please update your feature branch.

  • Since there is an additional build step before deploying the code to Zapier, please update .github/workflows/zapier-release.yml and add npm run build

    From this:

    - name: Deploy to Zapier
    env:
    ZAPIER_DEPLOY_KEY: ${{ secrets.ZAPIER_DEPLOY_KEY }}
    run: zapier push

    To this:

      - name: Deploy to Zapier
        env:
          ZAPIER_DEPLOY_KEY: ${{ secrets.ZAPIER_DEPLOY_KEY }}
        run: npm run build && zapier push

@nicomiguelino nicomiguelino requested a review from Copilot March 19, 2025 17:27
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the project from JavaScript to TypeScript while replacing Jest with Vitest and updating related configs and scripts.

  • Converts all JS files to TypeScript with appropriate type annotations and ES module import style
  • Replaces Jest configurations with Vitest and removes now-unused Jest files
  • Updates build scripts, linters, and documentation to reflect the migration

Reviewed Changes

Copilot reviewed 34 out of 36 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/index.ts Migrated to TypeScript using ES module imports
.eslintrc.cjs Removed Jest-specific environment settings
src/constants.ts Re-implemented constants as TypeScript exports
.github/workflows/test.yml Disabled linting step to bypass process blocking during testing
Various files in src/actions, triggers, and utils.ts Updated require statements to ES module imports, added type annotations
docs/developer-documentation.md Updated testing framework documentation
constants.js & jest.visual.config.js Removed obsolete JS config files as part of the migration process
Files not reviewed (2)
  • babel.config.json: Language not supported
  • package.json: Language not supported
Comments suppressed due to low confidence (1)

src/utils.ts:27

  • The polling loop in the waitForAssetReady function currently lacks a delay, which can lead to excessive requests. Consider adding a delay (e.g., using a sleep function or setTimeout) within the loop.
do {

hereje and others added 9 commits March 19, 2025 18:42
fix typo renaming import

Co-authored-by: Nico Miguelino <nicomiguelino2014@gmail.com>
update reference to renamed import

Co-authored-by: Nico Miguelino <nicomiguelino2014@gmail.com>
update release workflow so that it builds the
project before publish
update test workflow so that it works as expected

- update step "Run tests with mocks"
- udpate step "Run visual tests"
- add typescript compilation step
@hereje hereje force-pushed the feat/migrate-to-typescript branch from 5e73698 to 355d072 Compare March 20, 2025 01:23
Copy link
Copy Markdown
Collaborator

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

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

@hereje, please check the logs in this CI run.

npx vitest --reporter=json --outputFile=./test-output.json
  • The snippet above might help you adjust the workflow for coverage and test reports.

- solve issue with json-summary test mock step
- solve issue with visual test step
@hereje
Copy link
Copy Markdown
Contributor Author

hereje commented Mar 20, 2025

@hereje, please check the logs in this CI run.

npx vitest --reporter=json --outputFile=./test-output.json
  • The snippet above might help you adjust the workflow for coverage and test reports.

Thanks for your help. I have updated the test workflow.
Looking forward for your comments.

@nicomiguelino
Copy link
Copy Markdown
Collaborator

Thanks, @hereje! Great work.

I tried to test if the Zaps (that's what Zapier call their workflows) and works fine. With the recent changes that you just pushed, I'll do an E2E test again to see if it still works. I'll review the code again and will let you know if there are comments/nitpicks.

@nicomiguelino nicomiguelino requested a review from Copilot March 20, 2025 15:41
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the project from JavaScript to TypeScript while also replacing Jest with Vitest, updating workflows, build scripts, and documentation.

  • Convert all JS source files to TypeScript with minimal type changes (using "any" for now).
  • Update GitHub workflow files to include additional Node.js versions and modify test/visual test commands for Vitest.
  • Remove obsolete Jest configuration and update developer documentation accordingly.

Reviewed Changes

Copilot reviewed 36 out of 38 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
.github/workflows/lint.yml Added a linting workflow for push and pull request events.
src/index.ts Updated to TS; imports still reference ".js" files.
src/constants.ts Added TS constants replacing the old JS constants.
.github/workflows/test.yml Updated Node.js versions and test commands to suit Vitest usage.
.eslintrc.cjs Removed jest environment as part of migration.
.github/workflows/zapier-release.yml Updated release workflow to build before deploying.
All action and trigger files Migrated from require to ES module imports with TS annotations.
docs/developer-documentation.md Updated testing instructions and build steps for TypeScript.
index.js Now exports the compiled TS code from the dist folder.
jest.visual.config.js Removed obsolete Jest visual test config.
Files not reviewed (2)
  • babel.config.json: Language not supported
  • package.json: Language not supported
Comments suppressed due to low confidence (2)

src/index.ts:1

  • The import paths in index.ts still reference '.js' extensions while the source files are now written in TypeScript; please confirm that your build configuration handles these extensions appropriately.
import uploadAsset from './actions/upload-asset.js';

.github/workflows/test.yml:74

  • Similar to the other test command, the flag '--outputFile.json=' in the visual tests command is unusual; please ensure it follows the expected syntax of your testing framework.
CHROME_PATH="$(which chrome)" xvfb-run --auto-servernum --server-args="-screen 0 1280x800x24" npm run test:visual:ci -- --update --reporter=json --outputFile.json=visual-test-results.json

- remove extra `.json` in flag outputFile
Copy link
Copy Markdown
Collaborator

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

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

Some general comments

  • Please make sure to update your feature branch.
  • Please make sure that commits must have verified signatures in future PRs.

@nicomiguelino nicomiguelino merged commit c5eb9f4 into Screenly:master Mar 21, 2025
5 checks passed
@hereje hereje deleted the feat/migrate-to-typescript branch March 22, 2025 02:30
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.

[CHORE] Migrate to TypeScript

3 participants