-
Notifications
You must be signed in to change notification settings - Fork 508
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
Comments
I'm in favor of this for what it's worth. |
This comment has been minimized.
This comment has been minimized.
Hey, sorry I missed this preexisting issue! 😬 I already went ahead and extracted the eslint config into eslint-config-tsdx on npm. BTW, I My motivation was simply to use the wonderful tsdx eslint config as a standalone, for situations where I don't need the
|
@agilgur5 I'm not sure I understand the issue as described..
Are you sure that this is true for |
@agilgur5 Can we begin using That would mean
e.g. Instead of this: 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' } } : {},
};
|
@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 For example, if they have Having an extra package like |
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 |
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
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 frequently breaking part is one reason why (among others) I'm not particularly keen on publishing
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.
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
Given the above, which I mentioned in my opening statement, it is almost certainly breaking. But tests are good to have as well. |
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 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
This is exciting!
I don't consider this a serious issue but... According to semver, version
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
So every part of tsdx (linting/building/testing) can be useful in JS projects. Interesting, and good to know!
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.
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.
Oops! Thanks for taking a peek. I reviewed all the dependencies and made a couple fixes and published |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I've read the 5 LoC before
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
"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.
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.
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.
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.
I still have only taken a glance, but there's still a mismatch that TSDX does not support the entire
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 A |
Parcel also seems to be doing the same thing as this proposal in Parcel 2: parcel-bundler/parcel#3216 (comment) |
Just spitballin but would it possibly be a non-breaking (less-breaking) change to instead provide an eject command similar to CRA? |
@mimshwright per #652 / #389 (comment), |
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 betweenbabelrc
use in commands likebuild
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 overridingrollup-plugin-babel
intsdx.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
, andjest.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
(oreslint-plugin-tsdx
that also has a config for easier installation), andjest-preset-tsdx
. Or their equivalent@tsdx/
versions.Also add a default
.babelrc.js
,.eslintrc.js
, andjest.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 totsdx 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 ofbabel-preset-env
. Some things can be configured by just allowing users to pass options into the preset, but those won't be dynamic or reflecttsdx 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 withtsdx.config.js
, buttsdx lint
andtsdx 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 #289Per #634 (comment), Parcel 2 seems to do this for its Babel support as well
The text was updated successfully, but these errors were encountered: