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

Suggestion for JS workspace improvements #27

Open
barak007 opened this issue Sep 27, 2023 · 2 comments
Open

Suggestion for JS workspace improvements #27

barak007 opened this issue Sep 27, 2023 · 2 comments

Comments

@barak007
Copy link
Contributor

barak007 commented Sep 27, 2023

Assume lerna stays.

Task: remove babel
Reason: not needed anymore

Task: delete package-lock.json from all packages keep only one at the js root folder.
Reason: It is not necessary to have lock file in each package when you have a workspace

Task: move all devDependencies from each package.json to the root package.json.
Reason: it makes maintains/updating/install much simpler and keep the infra unified

Task: avoid .sh scripts and convert them to .js
Reason: better cross platform (Windows) support

Task: move tsconfig to the js root and extends each package
Reason: unify the configuration between packages, also since the build does not uses typescript the tsconfig is only used by the IDE.

Task: add prettier
Reason: enforce similar code format

let me know what if this sounds reasonable.

@nick-thompson
Copy link
Contributor

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 the js 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 each package.json to the root package.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 the js root and extends each package
Reason: unify the configuration between packages, also since the build does not uses typescript the tsconfig 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!

@barak007
Copy link
Contributor Author

Hey good to hear.

First you can see the preview for these changes in #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.

  • if there is a need for customizing the Typescript build config it will be done via the bundler config.
  • override tsconfig can be done with "extends" of the base config.

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
      }
    }
  ]
}

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

No branches or pull requests

2 participants