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

[RFC] Presets #634

Open
agilgur5 opened this issue Mar 23, 2020 · 17 comments
Open

[RFC] Presets #634

agilgur5 opened this issue Mar 23, 2020 · 17 comments
Labels
abusive contains abusive content kind: discussion Discussion on changes to make scope: integration Related to an integration, not necessarily to core (but could influence core)

Comments

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 23, 2020

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:
#110 (comment)
#389 (comment)

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

Per #634 (comment), Parcel 2 seems to do this for its Babel support as well

@kylemh
Copy link
Contributor

kylemh commented Mar 30, 2020

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

@Bnaya

This comment has been minimized.

@zenflow
Copy link

zenflow commented Aug 13, 2020

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:

  • a web application using the Next.js framework. It has it's own build system (which supports TypeScript), so I don't need tsdx build, and I don't always write unit tests for web apps, so I don't always need tsdx test. (For top-level software, or "apps", I'm more likely to do integration tests, with cypress.)
  • JavaScript (non-TypeScript) packages. No need for tsdx build obviously, and tsdx's configuration for Jest does not apply or make sense here.

@zenflow
Copy link

zenflow commented Aug 13, 2020

@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
Copy link

zenflow commented Aug 13, 2020

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

That would mean

  • transferring the eslint-config-tsdx package & repo to the formium organization (& I think ideally publishing v1.0.0)
  • replacing all of tsdx's dependencies for linting with the eslint-config-tsdx package
  • changing the .eslintrc.js template to use that one single eslint config

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
Copy link

zenflow commented Aug 13, 2020

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
Copy link

zenflow commented Aug 13, 2020

Re. my proposal (#634 (comment))

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
Copy link
Collaborator Author

agilgur5 commented Aug 14, 2020

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
Copy link

zenflow commented Aug 17, 2020

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

This comment has been minimized.

@agilgur5

This comment has been minimized.

@zenflow

This comment has been minimized.

Repository owner locked as too heated and limited conversation to collaborators Aug 17, 2020
@agilgur5 agilgur5 added the abusive contains abusive content label Aug 17, 2020
@agilgur5

This comment has been minimized.

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Aug 17, 2020

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 #428 (comment) 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 agilgur5 added the scope: integration Related to an integration, not necessarily to core (but could influence core) label Aug 19, 2020
@agilgur5 agilgur5 unpinned this issue Aug 22, 2020
Repository owner unlocked this conversation Sep 17, 2020
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Oct 2, 2020

Parcel also seems to be doing the same thing as this proposal in Parcel 2: parcel-bundler/parcel#3216 (comment)

@mimshwright
Copy link

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

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Oct 27, 2020

@mimshwright per #652 / #389 (comment), 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abusive contains abusive content kind: discussion Discussion on changes to make scope: integration Related to an integration, not necessarily to core (but could influence core)
Projects
None yet
Development

No branches or pull requests

5 participants