jaredpalmer / tsdx

Zero-config CLI for TypeScript package development
https://tsdx.io
MIT License
11.25k stars 508 forks source link

[RFC] Presets #634

Open agilgur5 opened 4 years ago

agilgur5 commented 4 years ago

Current Behavior

Right now, TSDX merges in any external configs with its own, i.e. users with babelrc, eslintrc, jest.config.js, etc don't have full agency over their configs if they want to opt-out of something.

The most confusion this has caused is with tsdx lint, e.g. #514, #498 . Linting tends to very project-specific, and when the rules don't line up 1-to-1 with the config (because it's merged), this is very noticeable and the solution is not intuitive.

The merge behavior also causes problems with Editor / IDE integrations, which has particularly impacted tsdx test as with #270 and its many duplicates and related issues.

This hasn't seemed to have affected tsdx build / .babelrc as much, but can be noticeable in issues like #383, where there have been inconsistencies between babelrc use in commands like build vs. test, as well as #543 where users are unable to change plugin order to that which they need and are stuck with whatever we choose because they can't opt-out easily either (without overriding rollup-plugin-babel in tsdx.config.js).

This merge behavior is not necessarily that uncommon, but some zero-config libraries do use presets instead as well.

Desired Behavior

Allow full overridability of .babelrc, .eslintrc, and jest.config.js when a user wants to do. Make integrations with existing editor tools more straightforward.

Suggested Solution

Add babel-preset-tsdx, eslint-config-tsdx (or eslint-plugin-tsdx that also has a config for easier installation), and jest-preset-tsdx. Or their equivalent @tsdx/ versions.

Also add a default .babelrc.js, .eslintrc.js, and jest.config.js to all the templates that just use these presets and nothing else to make for instant editor integration.

The ONE issue with this is that the internal Babel config merge is a bit more complicated than a plain merge; it does some custom configuration of preset-env and a few plugins are dynamic based on the options passed to tsdx build. So we'll have to leave some of that custom merging behavior in and may need to account for the case of e.g. babel-preset-tsdx instead of babel-preset-env. Some things can be configured by just allowing users to pass options into the preset, but those won't be dynamic or reflect tsdx build. Still need to think more on that specific case, will probably be more clear once the implementation is underway.

Could also add a tsconfig.json preset as well, which would make things like #504 not quite as breaking, but might not be so wanted as these get configured more frequently.

Who does this impact? Who is this for?

Any users with custom Babel, ESLint, or Jest configs. Any users of the default editor integrations with those.

NOT users who don't configure and NOT users who don't use those editor integrations.

Describe alternatives you've considered

There isn't another way to give full agency to users and that's kind of the problem. Well tsdx build can be mostly overridden with tsdx.config.js, but tsdx lint and tsdx test cannot.

This is the reason that this change would be a fairly BREAKING change, as people who used those configs with merging would suddenly stop getting merged and would have to switch to using the preset instead.

Editor integrations can be customized, but we want a more zero-config out-of-the-box experience for editor integrations too.

Additional context

Something I've been considering for a while as it's popped up in a lot of places for different parts of the codebase. Have linked a number of issues above.

Some other past references to having presets: https://github.com/jaredpalmer/tsdx/issues/110#issuecomment-521290271 https://github.com/jaredpalmer/tsdx/issues/389#issuecomment-568866380

There is also precedent from kyt, which uses presets now and is a package Jared imitated when creating TSDX per #289

Per https://github.com/formium/tsdx/issues/634#issuecomment-702889957, Parcel 2 seems to do this for its Babel support as well

kylemh commented 4 years ago

I'm in favor of this for what it's worth.

Bnaya commented 4 years ago

For the reference, neutrino is a preset based tools for kinda doing so https://github.com/neutrinojs/neutrino

zenflow commented 4 years ago

Hey, sorry I missed this preexisting issue! 😬

I already went ahead and extracted the eslint config into eslint-config-tsdx on npm. BTW, I am totally willing and hoping to will (whenever you want) transfer the package name to this project.

My motivation was simply to use the wonderful tsdx eslint config as a standalone, for situations where I don't need the build or test functions, such as:

zenflow commented 4 years ago

@agilgur5 I'm not sure I understand the issue as described..

[...] users with babelrc, eslintrc, jest.config.js, etc don't have full agency over their configs if they want to opt-out of something

Are you sure that this is true for eslintrc? (Sorry if I'm missing something!) It seems that the .eslintrc.js generated by tsdx lint --write-file has all the configuration that is necessary, and tsdx has no need to merge it with any additional configuration?

zenflow commented 4 years ago

@agilgur5 Can we begin using eslint-config-tsdx here?

That would mean

e.g. Instead of this:

https://github.com/formium/tsdx/blob/5e5c3f804e68609083be978a080ab905dc7ec19d/src/createEslintConfig.ts#L19-L31

We have this:

  const config = {
    extends: [
      'tsdx',
    ],
    settings: {
      react: {
        // Fix for https://github.com/jaredpalmer/tsdx/issues/279
        version: isReactLibrary ? 'detect' : '999.999.999',
      },
    },
  };

Or, nicer for users that aren't using react

  const config = {
    extends: [
      'tsdx',
    ],
    // Fix for https://github.com/jaredpalmer/tsdx/issues/279
    settings: isReactLibrary ? { react: { version: 'detect' } } : {},
  };

eslint-config-tsdx overrides version with '999.999.999' (i.e. "latest"), so non-react users don't need to define config.settings.react.version.

zenflow commented 4 years ago

Add babel-preset-tsdx, eslint-config-tsdx (or eslint-plugin-tsdx that also has a config for easier installation), and jest-preset-tsdx. Or their equivalent @tsdx/ versions.

@agilgur5 Regarding "or eslint-plugin-tsdx that also has a config for easier installation".. I believe you're referring to a workaround for eslint issue 3458, and by "easier installation" you mean "not needing to install a ton of peer dependencies".

See the warning regarding this in the eslint-config-tsdx README.

TL;DR It's fine for the eslint config package itself to include these dependencies, as long as the package ultimately using it doesn't have any different versions of those packages installed.

This is already the case for current users of tsdx lint. edited If their package directly depends on a different version [of one of the packages], eslint will use their version instead of the "correct" version we included in package dependencies. Or, if one of their dependencies depends on a different version [of one of these packages], eslint will fail when trying to resolve the plugin.

For example, if they have prettier or eslint-plugin-prettier in their package dependencies with a version range indicating a different version than the version we want, their version will be used instead of the version we want. Or, worse, if one of their dependencies depends on a different version, eslint will fail completely when trying to resolve the plugin.

Having an extra package like eslint-plugin-tsdx (if I understand it's idea correctly) won't fix that.

zenflow commented 4 years ago

Re. my proposal (https://github.com/formium/tsdx/issues/634#issuecomment-673736473)

I believe this would be a non-breaking change, as long as the actual eslint configuration and it's dependencies do not change in the transition. I was careful about this and someone can double-check.

@agilgur5 Should I go ahead with a PR? I'd love to see eslint-config-tsdx taken over by and integrated into the main project.

agilgur5 commented 4 years ago

I already went ahead and extracted the eslint config into eslint-config-tsdx on npm. BTW, I ~am totally willing and hoping to~ will (whenever you want) transfer the package name to this project.

Cool. Well only problem is that I was planning on monorepo'ing all the presets as they are quite tightly coupled right now and was planning to use @tsdx/eslint-config. This config is tiny, I haven't done so myself as it hasn't been top priority (nor have I had time) as this and related issues haven't gotten much upvotes. The monorepo pre-req is also a non-trivial amount of work (which I somewhat started when I built out the integration test suite).

tsdx eslint config as a standalone, for situations where I don't need the build or test functions, such as:

Yea my plans with this and splitting TSDX into more packages generally, ideally individual Babel and Rollup plugins, is to be able to handle decoupled scenarios like this as well as more easily extend to other use-cases beyond libraries, e.g. Node apps like requested in #654 . The decoupling may not be what Jared originally envisioned, though the super tight coupling right now causes lots of bottlenecks, including breaking changes of one dependency causing breaking changes for TSDX generally because it's all one big API right now.

  • & I think ideally publishing v1.0.0

The frequently breaking part is one reason why (among others) I'm not particularly keen on publishing v1.0.0s for any TSDX packages. 0.x can have breaking changes in minors.

  • JavaScript (non-TypeScript) packages. No need for tsdx build obviously, and tsdx's configuration for Jest does not apply or make sense here.

That's not necessarily true. TSDX supports JS as well -- I made several PRs to make that more possible and for easier gradual migrations of JS -> TS. TS can also typecheck JS as well to a limited extent. The Rollup bundling can be used for JS too.

Are you sure that this is true for eslintrc? (Sorry if I'm missing something!)

Yes, I think you're missing the second paragraph of my opening comment. I linked several issues in the first section, the two ESLint ones being #514 and #498

I believe this would be a non-breaking change, as long as the actual eslint configuration and it's dependencies do not change in the transition. I was careful about this and someone can double-check.

Given the above, which I mentioned in my opening statement, it is almost certainly breaking. But tests are good to have as well. I didn't fully check your dependencies, but even at a glance I can see those have some different versions -- TSDX has yet to upgrade to Prettier v2 (#632 ), which is a breaking change (and requires some other updates like TS 3.8 which isn't fully supported in Rollup).

zenflow commented 4 years ago

Cool. Well only problem is that I was planning on monorepo'ing all the presets as they are quite tightly coupled right now and was planning to use @tsdx/eslint-config. This config is tiny, I haven't done so myself as it hasn't been top priority (nor have I had time) as this and related issues haven't gotten much upvotes. The monorepo pre-req is also a non-trivial amount of work (which I somewhat started when I built out the integration test suite).

I don't know about the babel & jest presets, but the eslint preset seems quite loosely coupled with the others. It's essentially just a great eslint config with great support for typescript, babel & react. If you look at it's content, it's literally eslint-config-react-app but (1) enhanced with prettier (2) with react made optional.

So I think at least the eslint config package does not necessarily need to be part of a monorepo, although it might be desired for uniformity if/when the other config packages live in a monorepo, at which point it (the eslint config package) can be imported into the monorepo.

Regarding the package name: I may put in a PR using the eslint-config-tsdx since that package is published already, but it can be changed.


Yea my plans with this and splitting TSDX into more packages generally, ideally individual Babel and Rollup plugins, is to be able to handle decoupled scenarios like this as well as more easily extend to other use-cases beyond libraries, e.g. Node apps like requested in #654 .

This is exciting!


The frequently breaking part is one reason why (among others) I'm not particularly keen on publishing v1.0.0s for any TSDX packages. 0.x can have breaking changes in minors.

I don't consider this a serious issue but... What's wrong with the post-version-1 part of semantic versioning? I.e. What's the advantage of having "breaking changes in minors" instead of using major releases for breaking changes?

According to semver, version 0.x is intended for initial development:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

The practical advantage of moving past major version zero is that users can then receive minor & patch updates that should not be breaking. For example, if the last version of a package was 0.7.1 and version 0.7.2 is released with no breaking changes, I have no indication (without reading the release notes) that there were no breaking changes. In contrast, if the last version was 7.1.0 and version 7.2.0 is released, I can expect it to be backwards compatible, without needing to look at the release notes.


That's not necessarily true. TSDX supports JS as well -- I made several PRs to make that more possible and for easier gradual migrations of JS -> TS. TS can also typecheck JS as well to a limited extent. The Rollup bundling can be used for JS too.

So every part of tsdx (linting/building/testing) can be useful in JS projects. Interesting, and good to know!


Yes, I think you're missing the second paragraph of my opening comment. I linked several issues in the first section, the two ESLint ones being #514 and #498

Ok, thanks. I had originally looked at those issues and couldn't figure out what problem y'all were talking about, but I think now after a bit more digging into those issues (and the source code) I understand that issue.


Given the above, which I mentioned in my opening statement, it is almost certainly breaking. But tests are good to have as well.

I'm not sure what you're specifically referring to with "given the above"... I can't find anything you said that implies that the changes I proposed would be breaking. Yes, you made it clear that the changes you proposed would be breaking, but I was saying the changes that I proposed would be non-breaking.

I maintain that my proposed changes would be non-breaking "as long as the actual eslint configuration and it's dependencies do not change in the transition".

To be more clear about what I'm proposing, I'll open a PR. It's a pretty small & simple change anyways.


I didn't fully check your dependencies, but even at a glance I can see those have some different versions -- TSDX has yet to upgrade to Prettier v2 (#632 ), which is a breaking change (and requires some other updates like TS 3.8 which isn't fully supported in Rollup).

Oops! Thanks for taking a peek. I reviewed all the dependencies and made a couple fixes and published v0.2.0.

zenflow commented 4 years ago

@agilgur5 In response to your comment https://github.com/formium/tsdx/issues/801#issuecomment-674642249:

Duplicate of #634 ... Please don't knowingly make duplicate issues, they confuse people and unnecessarily take more time to respond to...

I did not (and would not) intentionally make a duplicate issue. The issue I opened describes a totally different problem than this one, or #514/#498, or any other I found when looking at preexisting issues. Sure it has some overlap with this issue in the proposed solutions, but as it's own problem with it's own (different) requirements for a solution, it could use it's own issue. Doesn't it (my issue) have the same relationship to this issue as #514 does, in that both (my issue & #514) are practical issues that would be covered by the solution you describe in this RFC?

edit I obviously think it should be unlocked and reopened, but if it's not, whatever I don't care. I just don't like the accusation that I would intentionally/knowingly make duplicate issues 😛

agilgur5 commented 4 years ago

@agilgur5 In response to your comment #801 (comment):

You know, I locked that issue for a reason... Responding to my comments there in a different thread is not exactly an appropriate or warranted response...

The issue I opened describes a totally different problem than this one

They're not... You even said so in your first comment... The title of your duplicate is a literally a subset of the title here: an ESLint Shareable Config is one of the proposed presets...

Doesn't it (my issue) have the same relationship to this issue as #514 does, in that both (my issue & #514) are practical issues that would be covered by the solution you describe in this RFC?

For one thing, there's a very relevant timeline here, where those are older issues than this one while yours is newer than this one... The second is they do not have the same relationship. #514 has multiple possible solutions, one of which is proposed here, one of which requires upstream work with an upstream issue I linked there, and there are more than that. The ideal solution actually requires both (meaning this doesn't resolve everything). #514 also causes a lot of confusion, so readers finding the workarounds listed there is intentional. As I've said above and as you did in your first comment, #801 is a subset of what is already here and your proposal is a partial implementation of this.


I locked that thread for a reason and I'm going to hide this response and your response. It is needlessly unproductive and tiring to debate minutiae about what it is literally a 5 LoC config. Responding to issues is very time-consuming already and duplicates add to that work unnecessarily. Please recognize the non-trivial amount of time I'm spending responding here. I will lock this thread too if necessary due to unproductive and off-topic comments.

zenflow commented 4 years ago

Responding to my comments there in a different thread is not exactly an appropriate or warranted response...

@agilgur5 The appropriate place to respond would have been there in that issue, but I couldn't because you locked it! What do you expect when you drop an insulting accusation on me like that and then lock the conversation? What was the point of locking it anyway? If you're not interested in an issue in your repo, you really don't have to be such a dick about it. There are so many options: You can just completely ignore it. You can close it without locking it. You can close it without comment. You can close it with a comment that doesn't include any insults. So many options.

They're not... You even said so in your first comment... The title of your duplicate is a literally a subset of the title here: an ESLint Shareable Config is one of the proposed presets...

Dude whatever, you're wrong (about those first 2 things) but you don't want to see how you're wrong so, whatever.

Have a nice morning.

You can delete this pointless thread whenever.

agilgur5 commented 4 years ago

Dude whatever, you're wrong

Sure, guess you know the intent of my issue better than me. And guess you know the issue landscape better than me and how to triage issues better than me even though I respond to all of them.

Btw, when you made inaccurate statements (like on JS usage), I didn't call you names for doing so.

What was the point of locking it anyway?

To avoid pointless debate like this. I responded to your comment here just to be respectful, but based on your abusive comment, you're clearly not respecting me and I guess it was a mistake to respond to you...

You can just completely ignore it.

No, I can't, that's bad maintenance. A good number of people appreciate that I respond quickly to most issues and triage everything, but I guess you don't.

You can close it without comment.

That's worse than ignoring. Somehow I don't think you'd take kindly to that either.....

You can close it with a comment that doesn't include any insults.

All I did was mark your issue as a duplicate. That's not an insult. Sorry for triaging issues, my bad, guess I'm a dick for volunteering my time to this repo. I appreciate the thought for my time, makes all the hours I spend a week on this worthwhile.

I marked your issue as a duplicate and your response was to personally attack me and call me names. I was performing my duties as a maintainer, triaging, scoping, and prioritizing work, and you insulted me for it.

agilgur5 commented 4 years ago

If you look at it's content

I've read the 5 LoC before

but the eslint preset seems quite loosely coupled with the others

It is looser than the others, but there's still coupling because TSDX is currently meant to control everything. Off the top of my head two examples are the react setting and the fact that Prettier 2.0 requires TS 3.8.

I'm not sure what you're specifically referring to with "given the above"...

"Given the above" refers to the issues I've linked to twice now from my opening statement. Per those issues, the current merging behavior is unoverrideable.

I can't find anything you said that implies that the changes I proposed would be breaking. Yes, you made it clear that the changes you proposed would be breaking, but I was saying the changes that I proposed would be non-breaking.

Ok, sure if you're limiting the changes to just a 3 LoC code extraction, then it might not be, but the dependency changes can also cause issues as they could be a depth of 2 in due to one of your changes while it was previously 1 (see below). These would need some integration testing to really get an idea.

Regarding "or eslint-plugin-tsdx that also has a config for easier installation".. I believe you're referring to a workaround for eslint issue 3458, and by "easier installation" you mean "not needing to install a ton of peer dependencies".

See the warning regarding this in the eslint-config-tsdx README.

TL;DR It's fine for the eslint config package itself to include these dependencies, as long as the package ultimately using it doesn't have any different versions of those packages installed.

Yes that is what I was referring to. I don't think that warning is a good idea -- ESLint explicitly does not sanction that approach and it breaks convention (one that many much larger configs follow), which would be confusing to users of multiple configs. You're relying on end-users to understand their dependency tree and treat your config differently from other configs. ESLint sanctions the "plugin with config" approach because of how namespacing works in plugins that re-export rules and that has a lot of real world usage. That comment was what I was specifically referring to.

This is already the case for current users of tsdx lint. edited

Yea that's a bug, see also https://github.com/formium/tsdx/issues/428#issuecomment-612545660 which refers to the upstream issue and the "plugin with config" workaround. It's not a super high priority and hasn't been upvoted as it's not hit often since users of TSDX stick to its lint rules generally. But if you're specifically opting out of TSDX and using a separate ESLint config, then that will likely increase incidence.

eslint.config.js is gaining traction so that's probably the ideal way to go anyway, so waiting for that to resolve a low priority upstream issue seems reasonable.

Oops! Thanks for taking a peek. I reviewed all the dependencies and made a couple fixes and published v0.2.0.

I still have only taken a glance, but there's still a mismatch that TSDX does not support the entire 1.x line, it's on ^1.19.1; same is true for the other dependencies. Looks like you also didn't add a changelog for that breaking downgrade. Should also add a deprecation message on the 0.1.x line that it had a dependency mismatch.

The practical advantage of moving past major version zero is that users can then receive minor & patch updates that should not be breaking. For example, if the last version of a package was 0.7.1 and version 0.7.2 is released with no breaking changes, I have no indication (without reading the release notes) that there were no breaking changes.

This is a completely unrelated issue that didn't really call for debate, but I'm well aware of how SemVer works and if you've seen my comments, PRs, and milestones in this repo and others, I am quite quick to point out breaking changes, try to make things backward compatible, and fix unintended breaking changes. Patch versions are not supposed to be breaking in the 0.x scheme either, but they may include additions to the API as a x.y minor normally would. As a contributor, that wasn't always the case and did cause issues and anger from other users, which is another reason I'm trying to rigorous and transparent as a maintainer.

A 1.x release means users can expect stability. If breaking changes are happening frequently and the package is under significant development (TSDX is nowhere near feature complete and several existing PRs and RFCs like this one that add improvements require breaking changes), it is simply not accurate to say it is stable. The "road to 1.0" is still quite long. On top of that, it makes it more difficult to release breaking changes; if your majors are changing frequently that indicates instability and loses user confidence. Users also tend to lag behind on major updates, which is visible within TSDX and within this thread even (Prettier 2.0). If you make them frequently, the lag becomes increasingly greater. Majors leave folks behind, but backward compat creates tech debt; it's a conundrum that increases as the API surface of a library increases. When it's clear the API surface will change frequently in a 0.x, then this is a transparent match of expectations and in my experience the lag is not as large.

agilgur5 commented 3 years ago

Parcel also seems to be doing the same thing as this proposal in Parcel 2: https://github.com/parcel-bundler/parcel/issues/3216#issuecomment-662668621

mimshwright commented 3 years ago

Just spitballin but would it possibly be a non-breaking (less-breaking) change to instead provide an eject command similar to CRA?

agilgur5 commented 3 years ago

@mimshwright per #652 / https://github.com/formium/tsdx/issues/389#issuecomment-568785862, eject will not be added. Pseudo-forks in general create lots of issues. If one needs to make some changes to the internal behavior that can't be done with existing escape routes (quite a lot can be done with them), TSDX officially recommends using patch-package, as is written in the docs.