Clean up options & remove explicit function types

[IanVS]

This is a bit of a cleanup PR, and does two main things:

• Removes function types from src/types.ts, and defines the types directly on the functions themselves. This was a holdover from before the conversion of the project to typescript, when we had to have a separate types file, but it's backwards from how types are normally defined. It's clearer and more typesafe to define the types directly on the functions, which this PR does now.
• Improves the options that we present to prettier (for it to show in prettier --help, supposedly, though I'm not sure whether that actually works or not).

Note: this doesn't have any impact on the types that we expose to users (which is manually maintained in types/index.d.ts

[fbartho]

No blocker feedback! Feel free to consider and reject my comments.

[fbartho]

Explicitly setting the default values in all of the test cases prevents us from detecting an accidental change in the defaults.

If we’re inserting these flags into all the tests, I recommend we add an explicit test case to assert the defaults haven’t changed.

[IanVS]

Good callout. I think we already have a test that covers this:

prettier-plugin-sort-imports/src/utils/__tests__/normalize-plugin-options.spec.ts

Lines 93 to 114 in c3f531f

	test('it should set most defaults', () => {
	expect(
	examineAndNormalizePluginOptions({
	importOrder: DEFAULT_IMPORT_ORDER,
	importOrderParserPlugins: [],
	importOrderCaseSensitive: false,
	importOrderTypeScriptVersion: '1.0.0',
	filepath: __filename,
	}),
	).toEqual({
	hasAnyCustomGroupSeparatorsInImportOrder: false,
	importOrder: [
	BUILTIN_MODULES_REGEX_STR,
	THIRD_PARTY_MODULES_SPECIAL_WORD,
	'^[.]',
	],
	importOrderCombineTypeAndValueImports: true,
	importOrderCaseSensitive: false,
	plugins: [],
	provideGapAfterTopOfFileComments: false,
	});
	});

[fbartho]

I don’t feel strongly about this, but should this still have been a type import?

[IanVS]

Ah, yeah probably. But the existing state is such a mess. I tried adding verbatimModuleSyntax so we would have to use import type consistently, but it doesn't work great with commonjs.

I think some day we should think about emitting ESM, which is supported only in prettier 3+. That would be a breaking change, and there are lots of people still using prettier 2, but we should probably drop it eventually and modernize.

[fbartho]

ESM is such a mess. I’m trying to only use it, but I know tools under the hood flow some stuff through CommonJS and I can’t easily change them. I think things are 80% there… but supposedly that was the case 3 years ago, and that was manifestly wrong.

I’ve been using https://tsup.egoist.dev/ for work and a thing I noticed was that it can inject shims so CJS-styled code can work.
Would you be up for a dual-bundled build for a while? (Both ESM and CJS?) — that shouldn’t be a breaking change, and if the tooling is good enough, it shouldn’t be a concern for maintainability. (and if the tooling is bad, then… we probably want to know before dropping CJS)

[IanVS]

I'm not sure about dual-bundling at this point, I'm not sure it has much of a benefit over what we have today. I think that as a prettier plugin, we don't have to worry too much about esm/cjs interop, as that juggling act has to be handled by prettier itself today. And since users don't directly require or import our package (they just specify a string name in the plugins array), the output module system we use doesn't really affect them.

The only real reasons in my mind to think about going to ESM-only would be improved compatibility with our own dependencies (Babel 8 is ESM-only) and a nicer TypeScript authoring experience for us. The downside is that we would lose Prettier 2 support. That's unfortunate, but might be a tradeoff we should make eventually. But I think we can kick the can a bit further down the road for now. Or can you think of a reason we should make changes sooner rather than later?

[fbartho]

I don’t feel urgency to migrate this codebase to ESM only (and I’m mildly worried about the ecosystem stability issues we might trip across in that migration project — every year I’ve gotten burned by something like that).

Babel 8 is probably the biggest reason, and even that doesn’t really resonate as a top-of-mind urgent goal — presumably performance is the biggest reason to do so other than normal “gotta be on a supported version”-type maintainability reasons?.

What is the “nicer TypeScript authoring experience” about? Do you just mean babel-native type-stripping? IMO type stripping can get people into trouble unless you have a type-checking phase built-in, and once you have that phase built-in, you often loose the speed advantages.

[IanVS]

Yeah, exactly, I'm not in a rush either. I think we can keep things how they are for now, and when we feel confident we can migrate, maybe next year sometime.

As for TS authoring, I'd like to turn on verbatimModuleSyntax, but when doing so, we would need to use the older import foo = require("foo"); instead of the ES6-style imports. But as you point out, there's not a huge reason to turn on that flag so long as we're using TS itself to generate our output. The main thing would be enforcing import type when not using imports as values. That's not required, but I prefer being explicit.

[IanVS]

Thanks for the quick and thorough review!