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

fix(akita): support TS strict mode #1050

Merged
merged 2 commits into from
Feb 13, 2023

Conversation

Den-dp
Copy link
Contributor

@Den-dp Den-dp commented Jan 31, 2023

This PR makes the public API of Akita to be consumable in a strict typescript project (and without skipLibCheck).

Refs: #870


The first thing I've tried is to enable strict: true for this monorepo and fix them - but the amount of errors in such case is pretty significant 🥵

After that, I tried to fix only TS errors that I found while consuming akita@8.0.0 in my project. And such approach helped me to decrease the API surface to a couple of places, which was somehow fixed in this draft/spike #1050.

Interestingly, #1050 already allowed me to consume akita in Angular 15 project with a strict mode (and without skipLibCheck).

But unfortunately:
1. I'm not confident in these TS-related changes (don't have a big project to test on)
UPD: @qnku @nunoarruda confirmed that it works for them
2. I want to find a proper tsconfig configuration that will help with the reproduction of the initial issue for future contributors
UPD: I'm thinking of converting ng-playground into strict mode and switching it to consume a lib from dist (the last one does not oblige us to fix all akita strict issues but only d.ts related) as a separate PR

But any recommendations on these 2 subjects are welcome!

Also, I'm totally ok if someone wants to use my spike to fix this issue ❤️

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @Den-dp to sign the Salesforce.com Contributor License Agreement.

@Den-dp Den-dp mentioned this pull request Jan 31, 2023
@Den-dp Den-dp force-pushed the fix/support-strict-mode-spike branch from 2560e45 to 682f66e Compare February 5, 2023 11:42
@qnku
Copy link

qnku commented Feb 9, 2023

@Den-dp do you have any idea why the PR build fails?

When I run npm run build:all with your changes I do not get any errors ...

@Den-dp
Copy link
Contributor Author

Den-dp commented Feb 10, 2023

@qnku thanks for your interest!

TL;DR: CI fails cause of nx-related misconfiguration, so not really relevant (also I already have a fix ✔️ for that)

Where I need some help is in testing these changes on a real code with a strict mode enabled 😉

It's relatively simple:

# build akita and pack it
npx nx build akita
cd dist/packages/akita
npm pack

# install it in your project using full path to the datorama-akita-8.0.0.tgz
npm i C:\github\akita\dist\packages\akita\datorama-akita-8.0.0.tgz

But if you are really interested in "why it works on your machine and doesn't on CI" then here are my findings:

  • npm run build:all fails on CI because nx is not configured to build ng-entity-service and ng-router-store after akita (it tries to build them all in parallel which is not possible)
  • It might work on your PC cause you probably already have akita artifacts in dist folder so nx is able to build the remaining two libs (can repro it by running npm run build:all twice)

@qnku
Copy link

qnku commented Feb 10, 2023

@Den-dp Thanks for the quick update.

We have a medium sized application and use the stores almost everywhere.

I can confirm that the build errors we previously got with the 8.0.0 release and Angular 15 do not occur anymore with your fix. Also found no runtime errors during my tests (which I didn't expect anyway, since you just changed the typings).

For the test I used your installation instruction, turned skipLibChecks off again. Strict mode has always been on.

Very happy, hoping for a release soon :-)

@nunoarruda
Copy link

Hey @Den-dp, thanks for the effort 👍

I have tested your changes in an Angular 15 project with strict mode enabled. No more type/compilation errors. My stores of type Store and of type EntityStore are working fine.

Perhaps the CI/CD/build problem should be addressed in a different issue/PR. That's probably not directly related to the strict mode issue.

Do you want to remove the draft state of this PR and ping Netanel?

This PR makes the public API of Akita to be consumable in a strict typescript project

Refs: salesforce#870
@Den-dp Den-dp force-pushed the fix/support-strict-mode-spike branch from 682f66e to fb1aa61 Compare February 12, 2023 16:13
@Den-dp Den-dp marked this pull request as ready for review February 12, 2023 16:14
@Den-dp
Copy link
Contributor Author

Den-dp commented Feb 12, 2023

Thanks for your help @qnku @nunoarruda!

I've converted this Draft into PR, updated the description with your feedback, and also provided a CI fix.


Initially, I wanted to find out what is the best way to repro the issue using this repo, so that acceptance criteria for #870 would be more straightforward and obvious.

The only solution I came up with is to convert the ng-playground app from this repo into strict and switch it to use compiled libs from dist. The last one helps us by not obliging to fix all akita strict issues but only d.ts related (aka public API surface).

But the ng-playground is big too and requires more time so I will do it in the next PR

Add `dependsOn` for `build` to build dependencies of a project first before building the project
Remove "nx/presets/npm.json" to fix deps graph (cause it disabled the analyzeSourceFiles feature)
@Den-dp Den-dp force-pushed the fix/support-strict-mode-spike branch from fb1aa61 to 5610f55 Compare February 12, 2023 17:06
@NetanelBasal NetanelBasal merged commit 1d9eaac into salesforce:master Feb 13, 2023
@Den-dp Den-dp deleted the fix/support-strict-mode-spike branch February 13, 2023 11:28
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