Breaking: Use transformSync option for sync transform

[chrisblossom]

I think there should be a way to have a sync and async transform. Also, there isn't a way to properly type the function at its current state.

Options:

• Use options object { sync: () => 'transformSync', async: async () => 'transformAsync' }.
• Add transformSync option.
• Deprecate the direct transform option and add: transformSync / transformAsync options.
• No API changes. Uncomment out Promise<CosmiconfigResult> and use // @ts-ignore comments to suppress any typescript errors. (not my preference)

Fixes #197 (comment) via add transformSync option.

It might be better to have the object only format:

const transform = {
	sync?: (result) => result,
	async?: async (result) => result,
}

[codecov-io]

Codecov Report

Merging #209 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #209   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines         231    231           
  Branches       51     51           
=====================================
  Hits          231    231

Impacted Files	Coverage Δ
src/createExplorer.ts	100% <100%> (ø)	⬆️
src/index.ts	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c9adc1...a8e8b73. Read the comment docs.

[davidtheclark]

I don't think this is ideal, but also don't think it makes things worse. The core problem, I think, is that it's possible to configure things in a way that produces surprising results: Before this change, you could pass an async function to transform, then use sync loading functions (like searchSync()), and be surprised that things run async. After this change, you could only provide an async transform function, then use sync loading functions, and be surprised that your transform is not applied.

There's a similar problem with the way loaders work — you can get yourself in a sync/async tangle pretty easily.

Turns out providing both sync and async APIs is kind of a nightmare.

As long as we're considering breaking changes, here's a wild idea, if we want to be ambitious: What if we make the sync/async split at the very top level of the API? When you initialize cosmiconfig, you need to initialize a sync or async instance: cosmiconfig(options) or cosmiconfigSync(options). Then the options you provide and functions you use are neatly constrained to either sync or async, and you can't confusingly tangle them up. For a sync instance, your loaders and transform must be sync, and search and load run synchronously (we could still call then searchSync and loadSync for clarity). For an async instance, your loaders and transform could be sync or async, doesn't matter.

[olsonpm]

I like that idea and think it will fit the common use-case nicely.

then again I don't remember seeing any issues with this so maybe it's not a problem in practice ?

[chrisblossom]

What if we make the sync/async split at the very top level of the API?

I can't think of any downsides to this. Seems the easiest, least complicated, and smallest API solution.

we could still call then searchSync and loadSync for clarity

I personally don't think this is necessary. I think it is pretty clear/can be made more clear by the user if needed via variable names, but I don't have a strong opinion of this at all.

then again I don't remember seeing any issues with this so maybe it's not a problem in practice ?

I found the potential issues with transform and loaders when converting the codebase to typescript. The issues existed previously with flow, just wasn't very apparent with how they were typed. Currently can't be correctly typed (at least with my knowledge).

Very possible this is not an issue for anyone. My guess is the majority of people use cosmiconfig's sync API and almost always pass sync functions for transform and loaders.

But that being said, I still think it is better to fix these things now since there is already going to be a breaking release.

Just so we are on the same page, this is what I'm understanding the API to look like:

import { cosmiconfig, cosmiconfigSync } from 'cosmiconfig';

const explorer = cosmiconfig({
	loaders: {
		'.js': async () => {},
		'.ts': () => {}, // can be sync
	},
	transform: async (config) => config,
	// transform: (config) => config, // can also be sync
});

await explorer.search();
await explorer.load();

const explorerSync = cosmiconfigSync({
	loaders: {
		'.js': () => {},
		'.ts': async () => {}, // can not be async, invalid
	},
	transform: (config) => config,
	// transform: async (config) => config, // can not be async, invalid
});

explorerSync.search();
explorerSync.load();

[davidtheclark]

@chrisblossom Yep, that's right.

This is quite a bit of work; I don't know how soon I myself would get to it — happy to look at PRs. Meanwhile, I think I'd rather not make other breaking API changes if we think this is the best route. If nobody feels like implementing this change before we release v6, I think that's ok: we'll have minimal breaking changes in v6. But if somebody does feel like implementing this change now, great.

[chrisblossom]

I started working on this. Should have a PR ready sometime relatively soon.

EDIT: I've got this working, just need to figure out how to correctly type a couple functions and update tests / readme.

[chrisblossom]

Closed in-favor of #211.