Open barak007 opened 1 year ago
Hey @barak007 thank you for this! And I've looked through your PR for the same, I appreciate it. Some of these items I'm definitely on board with, some of them I'm not sure. Either way, I will definitely want to go in small steps here as we move forward; I think the current #28 tries to cover too many of these things at once for my preferences.
Let me respond to each task in particular–
Task: remove
babel
Reason: not needed anymore
Really? I'm excited to hear that! It was in there for something that jest
needed to run the tests effectively, but perhaps that's no longer relevant. I would love to take a PR just to get babel out
Task: delete
package-lock.json
from all packages keep only one at thejs
root folder. Reason: It is not necessary to have lock file in each package when you have a workspace
I get the idea, but why is workspaces better than lerna? This seems to me a bit like a "if it's not broken, don't fix it" situation. Lerna covers the same set of solutions, no?
Task: move all
devDependencies
from eachpackage.json
to the rootpackage.json
. Reason: it makes maintains/updating/install much simpler and keep the infra unified
Agree but at the same time I'm not sold: it also seems like it would make it harder for a given package to deviate from the configuration of the others if it needed to. There is a reason (unfortunately) that the different js packages use different build workflows, which makes me a bit hesitant to try to unify them too early
Task: avoid
.sh
scripts and convert them to.js
Reason: better cross platform (Windows) support
Definitely! Good catch, I'd love to take a PR for this one as well
Task: move
tsconfig
to thejs
root and extends each package Reason: unify the configuration between packages, also since the build does not uses typescript thetsconfig
is only used by the IDE.
I'm interested in this change, and also a bit confused– the builds do use typescript. Or do you mean that the packages ultimately ship as js with .d.ts
files?
Task: add prettier Reason: enforce similar code format
I'm on board! For this one I'd say let's start by aligning on a config, then I'd like to approach the actual change set with 1 commit including the config files and a second commit which actually runs prettier and commits the result (imo this makes it clear when trying to follow the git history exactly what happened; you wouldn't need to go unearthing the config file from a single commit that touched many files)
Let me know what you think!
Hey good to hear.
First you can see the preview for these changes in https://github.com/elemaudio/elementary/pull/28 and I will also response one by one.
Really? I'm excited to hear that! It was in there for something that jest needed to run the tests effectively, but perhaps that's no longer relevant. I would love to take a PR just to get babel out
This is a semi big change which raises some type questions. In order to remove babel I used ts-jest
this is why I changed the test files to .ts
in the PR, this is why I found some type issues that I had to fix and also possible bug. (from that the questions)
I get the idea, but why is workspaces better than lerna? This seems to me a bit like a "if it's not broken, don't fix it" situation. Lerna covers the same set of solutions, no?
From my experience if you have npm workspace, lerna become unnecessary dependency as it's only giving you a good version bump mechanism over the workspace features of npm.
The version bump is valuable but you can still remove lerna from the deps tree
and use npx
to run it once you ready to release. it will also shrink the install time for anyone how does not releasing code.
Agree but at the same time I'm not sold: it also seems like it would make it harder for a given package to deviate from the configuration of the others if it needed to. There is a reason (unfortunately) that the different js packages use different build workflows, which makes me a bit hesitant to try to unify them too early
The way I see it is you have your tools in one place and you use/configure them differently in each package. It's not this or that you can still have both for extreme cases (which you don't have currently). I mean that you can still install a "special" dev dependency on a specific package but I don't see a case for that. this change is not for unifying the build/behavior of packages, just organize them in one place.
Definitely! Good catch, I'd love to take a PR for this one as well
Nice
I'm interested in this change, and also a bit confused– the builds do use typescript. Or do you mean that the packages ultimately ship as js with .d.ts files?
Currently this project does not build with tsc
it uses bundlers (rollup, tsup) and they handle the typescript from source. so the tsconfig
is solely for the ide.
This does not block per package overrides.
I'm on board! For this one I'd say let's start by aligning on a config, then I'd like to approach the actual change set with 1 commit including the config files and a second commit which actually runs prettier and commits the result (imo this makes it clear when trying to follow the git history exactly what happened; you wouldn't need to go unearthing the config file from a single commit that touched many files)
Sure, this is my standard .prettierrc
config for typescript. as you can see nothing special and the code look consistent. (I will not argue on any rule)
{
"printWidth": 120,
"singleQuote": true,
"overrides": [
{
"files": ["*.js", "*.cjs", "*.mjs", "*.ts", "*.tsx", "*.cts", "*.mts"],
"options": {
"tabWidth": 4
}
}
]
}
Assume
lerna
stays.Task: remove
babel
Reason: not needed anymoreTask: delete
package-lock.json
from all packages keep only one at thejs
root folder. Reason: It is not necessary to have lock file in each package when you have a workspaceTask: move all
devDependencies
from eachpackage.json
to the rootpackage.json
. Reason: it makes maintains/updating/install much simpler and keep the infra unifiedTask: avoid
.sh
scripts and convert them to.js
Reason: better cross platform (Windows) supportTask: move
tsconfig
to thejs
root and extends each package Reason: unify the configuration between packages, also since the build does not uses typescript thetsconfig
is only used by the IDE.Task: add prettier Reason: enforce similar code format
let me know what if this sounds reasonable.