extract "core" package, small improvements#137
Conversation
…ss pat-override to retryer, update snapshots
… --filter arguments
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
marcalexiei
left a comment
marcalexiei
left a comment
There was a problem hiding this comment.
Apart from this the PR also contains several smaller changes like fixing dependabot, upgrading dependencies, aligned "package.json" files, added tests, ...
Reviewing all changes is too much,
The Github UI is not very responsive after viewing 74 / 144.
I’ve left a few comments, but in my opinion this PR contains too many changes to be fully reviewed.
In the future, please let’s aim to keep PRs smaller to make the review process easier.
Understandable. I wasn't sure if anyone would review my changes, which is why I didn't pay so much attention to PR size. But it's great that you are willing to review, and I'll keep this in mind for next time. |
marcalexiei
left a comment
marcalexiei
left a comment
There was a problem hiding this comment.
Understandable. I wasn't sure if anyone would review my changes, which is why I didn't pay so much attention to PR size. But it's great that you are willing to review, and I'll keep this in mind for next time.
Great, we will eventually review part of this code when migrating core and backend to typescript!
...Also, I hope that we're done with these big refactorings soon.
I don't see any additional structure improvements that might lead to such big changes since we now have a clear distinction between packages and apps.
The only big refactor I could think of is migrating the core and the backend workspaces to typescript.
What are we going to do next?
Are we going to create a PR for building core package (dist with js and d.ts files) and squash it into this branch, so we can get rid of the type error?
There was a problem hiding this comment.
We have some lint errors in this file:
4:6 error Use an `interface` instead of a `type` @typescript-eslint/consistent-type-definitions
17:9 error Array type using 'string[]' is forbidden. Use 'Array<string>' instead @typescript-eslint/array-type
32:9 error Array type using 'string[]' is forbidden. Use 'Array<string>' instead @typescript-eslint/array-type
39:9 error Array type using 'string[]' is forbidden. Use 'Array<string>' instead @typescript-eslint/array-type
50:9 error Array type using 'string[]' is forbidden. Use 'Array<string>' instead @typescript-eslint/array-type
62:9 error Array type using 'string[]' is forbidden. Use 'Array<string>' instead @typescript-eslint/array-typeThere was a problem hiding this comment.
We have some lint errors in this file:
1:13 error Use an `interface` instead of a `type` @typescript-eslint/consistent-type-definitions
10:13 error Use an `interface` instead of a `type` @typescript-eslint/consistent-type-definitions
32:13 error Use an `interface` instead of a `type` @typescript-eslint/consistent-type-definitions
52:13 error Use an `interface` instead of a `type` @typescript-eslint/consistent-type-definitions
60:13 error Use an `interface` instead of a `type` @typescript-eslint/consistent-type-definitions
61:15 error Array type using 'T[]' is forbidden. Use 'Array<T>' instead @typescript-eslint/array-type
74:12 error Array type using 'T[]' is forbidden. Use 'Array<T>' instead @typescript-eslint/array-type
95:14 error Array type using 'T[]' is forbidden. Use 'Array<T>' instead @typescript-eslint/array-type
104:22 error Array type using 'T[]' is forbidden. Use 'Array<T>' instead @typescript-eslint/array-type
124:13 error Use an `interface` instead of a `type` @typescript-eslint/consistent-type-definitions
Yes. Except I planned to merge it instead of squashing. How do we approach this next step? Since you wrote "I can take care of generating the dist folder with the artifacts that will be published to npm (JavaScript and .d.ts files produced by tsc)", should I just wait for a PR from you? |
|
We have the CI broken so I would prefer to get it fixed before merge. |
2 participants
This PR mainly refactors the code by extracting a "core" package which is then used by the "backend" and "frontend" apps.
Apart from this the PR also contains several smaller changes like fixing dependabot, upgrading dependencies, aligned "package.json" files, added tests, ...
addresses step 1 of #32
closes #136