Add Support for Sorting Exports

[JCrepin]

• Off by default (enable via rule config)
• Only sorts exports after all imports
• Piggybacks off the existing import rules, with a few tweaks to get them to work with both imports and exports
• Basic unit tests included. I don't see anyone using these rules for much more than index files.

[lydell]

Hi!

Off by default (enable via rule config)

Do we really need an option? 🤔

Only sorts exports after all imports

What does this mean?

Piggybacks off the existing import rules, with a few tweaks to get them to work with both imports and exports

Yes, we should share as much as possible, but not do odd things like passing something that isn’t an import into isSideEffectImport.

Basic unit tests included. I don't see anyone using these rules for much more than index files.

What does this mean? We have to do the right thing regardless of how users use exports?

---

Is the idea to find all runs (chunks) of supported variants of export statements and sort them internally? Where a chunk of exports can be interrupted by imports, unsupported exports and other code.

Can you add all forms of exports to the tests? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export#Syntax. This way it should be clear which types of exports we sort and which we don’t, and that we don’t sort some by mistake.

Can using export … from cause side effects? Is this something we need to think about? Hopefully we can just mention it in the readme.

Can you look through some big open source projects and take examples of real-life exports from them? There are already some import tests “stolen” like this.

[JCrepin]

Hi,

Thanks for getting back to me so quickly.

Off by default (enable via rule config)

Do we really need an option? 🤔

This depends on whether we want this to be a breaking change or not. Without it, we're forcing the rule, which may cause friction for large code bases that don't want to update existing code. Thoughts?

Only sorts exports after all imports

What does this mean?

This means I'm only sorting exports that are after all the imports (everything prior is ignored). Actually, this should probably be removed, now that I think about it. It does lead into the question of if we'll want to introduce rules as to how exports sort in relation to imports though, as mentioned by @remcohaszing in #43. If you agree with those rules, I'll add them.

Piggybacks off the existing import rules, with a few tweaks to get them to work with both imports and exports

Yes, we should share as much as possible, but not do odd things like passing something that isn’t an import into isSideEffectImport.

Hmm, the alternative for this example is to add conditional logic to getItems (previously getImportItems) to only call this for imports, or to split getItems into getImportItems and getExportItems. I think returning false for isSideEffectImport is cleaner. Do you have a preferred approach?

Is the idea to find all runs (chunks) of supported variants of export statements and sort them internally? Where a chunk of exports can be interrupted by imports, unsupported exports and other code.

Yes, though as mentioned above, I'm only doing this after all imports, but it can be changed to process all exports in the file.

Can you add all forms of exports to the tests? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export#Syntax. This way it should be clear which types of exports we sort and which we don’t, and that we don’t sort some by mistake.

Can you look through some big open source projects and take examples of real-life exports from them? There are already some import tests “stolen” like this.

Both done.

Can using export … from cause side effects? Is this something we need to think about? Hopefully we can just mention it in the readme.

Yes they can. The only issue I could see is if you have two side-effect imports that need to be run in a specific order that's not alphabetical. If you need to export these, then you'd need to disable the rule so we don't sort them. If you don't need to export them, you'd just do side-effect imports in the appropriate order before your exports.

[lydell]

large code bases that don't want to update existing code.

Why wouldn’t they? (There might be a legit reason, I just can’t think of it.) One way would be to release a major version without the option and add it later if people complain convincingly.

I think returning false for isSideEffectImport is cleaner. Do you have a preferred approach?

I haven’t looked at the code close enough. You can leave it as-is.

I'm only doing this after all imports, but it can be changed to process all exports in the file.

Yes, I think we should sort exports not matter where they are in the file (drop the “only after imports” thing).

It does lead into the question of if we'll want to introduce rules as to how exports sort in relation to imports though

I think we should keep the idea where each “chunk” of sortable statements are sorted individually. If you want to merge chunks you need a different rule, such as import/first. Is there something like import/first but for exports?

The only question is if imports and exports can be in the same chunk, or if they should be different chunks. I don’t think I’d be against letting imports and exports be in the same chunk, turning this:

import a from "a"
export * as b from "b"
import c from "c"

into this:

import a from "a"
import c from "c"
export * as b from "b"

If so, the rule would be “exports always come after all imports in a chunk”.

One worry I have, though, is that this:

import a from "a"
export * as b from "b"
export const x = 5
import c from "c"

Would be unchanged (because export const breaks into two chunks, which are already sorted). This feels a little but unintuitive at first glance, and might be difficult to explain to users.

Maybe it makes sense to let chunks include any import statements plus any export statements? If so, the rule could be:

1. import statements, sorted by from
2. export statements with from, sorted by from
3. other export statements, in their original order

Could that cause problems, like evaluation order stuff? I don’t think so, but I’m not sure 🤔

---

Have I understood it correctly that we only support sorting export statements that have a from? That sounds good to me.

But do we also want to sort the things inside { and } in the following?

export { b, z as a, c };

We can’t move such an export statement itself, but we could sort the exported items.

[remcohaszing]

This means I'm only sorting exports that are after all the imports (everything prior is ignored). Actually, this should probably be removed, now that I think about it. It does lead into the question of if we'll want to introduce rules as to how exports sort in relation to imports though, as mentioned by @remcohaszing

I actually meant re-exports should be located somewhere consistently after sorting. I didn’t mean the original location of the re-exports.

I think code should always be sorted as imports first, then all re-exports, then anything else. I don’t think regular exports should be handled by this rule.

So the following code:

import a from "a" // import
export * as b from "b" // re-export
export const x = 5 // other
import c from "c" // import

// more other

would be sorted as:

import a from "a" // import
import c from "c" // import

export * as b from "b" // re-export

export const x = 5 // other
// more other

[lydell]

Good idea with the blank lines between import and export. But should there be one between those exports? 🤔 What’s the rule?

[remcohaszing]

I don’t take exports into account for this rule, only re-exports (export whatever from "module")

In the example, export const x = 5 is not a re-export, so it falls in the other category. It might as well have been a function call for example.

Exports may appear in other places as well and may depend on other statements. A more complete example:

import a from "a" // import
import c from "c" // import

export * as b from "b" // re-export

// anything below this line is unhandled
export const x = 5
console.log(a)
console.log(c)
export const y = x

export function foo() {
  console.log(x);
}
export const bar = foo;

This shows exports can’t be safely sorted. foo depends on x, but alphabetically it would go before x.

Only sorting exported const statements wouldn’t work either, in the example above bar must be defined after foo.

[lydell]

Yes, that’s in line with how things already worked and what I proposed.

[JCrepin]

One way would be to release a major version without the option and add it later if people complain convincingly.

I agree, so long as it's a major version bump, that works for me. The only reason someone wouldn't want to do this is the time it could take making the changes in large teams as they could run into merge conflicts. I've removed the config.

Yes, I think we should sort exports not matter where they are in the file (drop the “only after imports” thing).

Done.

I don’t think I’d be against letting imports and exports be in the same chunk
Could that cause problems, like evaluation order stuff? I don’t think so, but I’m not sure 🤔
Good idea with the blank lines between import and export. But should there be one between those exports? 🤔 What’s the rule?

As we keep the original order of source-less exports, I don't see any issues here. We allow source-less exports to be in a chunk, though I've ensured a newline between exports and source-less exports. For example:

export { w } from "w";
import { z } from "z";
export const b = 5;
export const a = b;
export { y } from "y";
export { x } from "x";

becomes:

import { z } from "z";

export { w } from "w";
export { x } from "x";
export { y } from "y";

export const b = 5;
export const a = b;

But do we also want to sort the things inside { and } in the following?

Yeah, it makes sense to sort the specifiers here. I've made this change, but obviously it's just the specifiers we sort in this case- the export itself doesn't get sorted.

[lydell]

Awesome work, I’ll have a look at it this weekend.

[lydell]

I haven’t had the time to look into this as much as I had wanted, but I think we have explored everything we need to in this PR. Thanks! This is now om my list of things to do, and I would like continue the work from here myself the upcoming weeks if you don’t mind?

[JCrepin]

I would like continue the work from here myself the upcoming weeks if you don’t mind?

By all means, go ahead. Thanks.

[lydell]

Hi! Just wanted to let you know that I haven’t forgotten about this. Made some progress today on updating the readme and examples.

[lydell]

Made some more progress on tests and review today.

[lydell]

Here’s an example where the output of eslint-plugin-simple-import-sort kind of is worse, I think:

    // https://github.com/graphql/graphql-js/blob/f7061fdcf461a2e4b3c78077afaebefc2226c8e3/src/utilities/index.js#L1-L115
    {
      code: input`
          |// @flow strict
          |
          |// Produce the GraphQL query recommended for a full schema introspection.
          |// Accepts optional IntrospectionOptions.
          |export { getIntrospectionQuery } from './getIntrospectionQuery';
          |
          |export type {
          |  IntrospectionOptions,
          |  IntrospectionQuery,
          |  IntrospectionSchema,
          |  IntrospectionType,
          |  IntrospectionInputType,
          |  IntrospectionOutputType,
          |  IntrospectionScalarType,
          |  IntrospectionObjectType,
          |  IntrospectionInterfaceType,
          |  IntrospectionUnionType,
          |  IntrospectionEnumType,
          |  IntrospectionInputObjectType,
          |  IntrospectionTypeRef,
          |  IntrospectionInputTypeRef,
          |  IntrospectionOutputTypeRef,
          |  IntrospectionNamedTypeRef,
          |  IntrospectionListTypeRef,
          |  IntrospectionNonNullTypeRef,
          |  IntrospectionField,
          |  IntrospectionInputValue,
          |  IntrospectionEnumValue,
          |  IntrospectionDirective,
          |} from './getIntrospectionQuery';
          |
          |// Gets the target Operation from a Document.
          |export { getOperationAST } from './getOperationAST';
          |
          |// Gets the Type for the target Operation AST.
          |export { getOperationRootType } from './getOperationRootType';
          |
          |// Convert a GraphQLSchema to an IntrospectionQuery.
          |export { introspectionFromSchema } from './introspectionFromSchema';
          |
          |// Build a GraphQLSchema from an introspection result.
          |export { buildClientSchema } from './buildClientSchema';
          |
          |// Build a GraphQLSchema from GraphQL Schema language.
          |export { buildASTSchema, buildSchema } from './buildASTSchema';
          |export type { BuildSchemaOptions } from './buildASTSchema';
          |
          |// Extends an existing GraphQLSchema from a parsed GraphQL Schema language AST.
          |export {
          |  extendSchema,
          |  // @deprecated: Get the description from a schema AST node and supports legacy
          |  // syntax for specifying descriptions - will be removed in v16.
          |  getDescription,
          |} from './extendSchema';
          |
          |// Sort a GraphQLSchema.
          |export { lexicographicSortSchema } from './lexicographicSortSchema';
          |
          |// Print a GraphQLSchema to GraphQL Schema language.
          |export {
          |  printSchema,
          |  printType,
          |  printIntrospectionSchema,
          |} from './printSchema';
          |
          |// Create a GraphQLType from a GraphQL language AST.
          |export { typeFromAST } from './typeFromAST';
          |
          |// Create a JavaScript value from a GraphQL language AST with a type.
          |export { valueFromAST } from './valueFromAST';
          |
          |// Create a JavaScript value from a GraphQL language AST without a type.
          |export { valueFromASTUntyped } from './valueFromASTUntyped';
          |
          |// Create a GraphQL language AST from a JavaScript value.
          |export { astFromValue } from './astFromValue';
          |
          |// A helper to use within recursive-descent visitors which need to be aware of
          |// the GraphQL type system.
          |export { TypeInfo, visitWithTypeInfo } from './TypeInfo';
          |
          |// Coerces a JavaScript value to a GraphQL type, or produces errors.
          |export { coerceInputValue } from './coerceInputValue';
          |
          |// Concatenates multiple AST together.
          |export { concatAST } from './concatAST';
          |
          |// Separates an AST into an AST per Operation.
          |export { separateOperations } from './separateOperations';
          |
          |// Strips characters that are not significant to the validity or execution
          |// of a GraphQL document.
          |export { stripIgnoredCharacters } from './stripIgnoredCharacters';
          |
          |// Comparators for types
          |export {
          |  isEqualType,
          |  isTypeSubTypeOf,
          |  doTypesOverlap,
          |} from './typeComparators';
          |
          |// Asserts that a string is a valid GraphQL name
          |export { assertValidName, isValidNameError } from './assertValidName';
          |
          |// Compares two GraphQLSchemas and detects breaking changes.
          |export {
          |  BreakingChangeType,
          |  DangerousChangeType,
          |  findBreakingChanges,
          |  findDangerousChanges,
          |} from './findBreakingChanges';
          |export type { BreakingChange, DangerousChange } from './findBreakingChanges';
          |
          |// Report all deprecated usage within a GraphQL document.
          |export { findDeprecatedUsages } from './findDeprecatedUsages';
      `,
      output: (actual) => {
        expect(actual).toMatchInlineSnapshot(`
          |// @flow strict
          |
          |// Produce the GraphQL query recommended for a full schema introspection.
          |// Accepts optional IntrospectionOptions.
          |// Asserts that a string is a valid GraphQL name
          |export { assertValidName, isValidNameError } from './assertValidName';
          |// Create a GraphQL language AST from a JavaScript value.
          |export { astFromValue } from './astFromValue';
          |export type { BuildSchemaOptions } from './buildASTSchema';
          |// Build a GraphQLSchema from GraphQL Schema language.
          |export { buildASTSchema, buildSchema } from './buildASTSchema';
          |// Build a GraphQLSchema from an introspection result.
          |export { buildClientSchema } from './buildClientSchema';
          |// Coerces a JavaScript value to a GraphQL type, or produces errors.
          |export { coerceInputValue } from './coerceInputValue';
          |// Concatenates multiple AST together.
          |export { concatAST } from './concatAST';
          |// Extends an existing GraphQLSchema from a parsed GraphQL Schema language AST.
          |export {
          |  extendSchema,
          |  // @deprecated: Get the description from a schema AST node and supports legacy
          |  // syntax for specifying descriptions - will be removed in v16.
          |  getDescription,
          |} from './extendSchema';
          |export type { BreakingChange, DangerousChange } from './findBreakingChanges';
          |// Compares two GraphQLSchemas and detects breaking changes.
          |export {
          |  BreakingChangeType,
          |  DangerousChangeType,
          |  findBreakingChanges,
          |  findDangerousChanges,
          |} from './findBreakingChanges';
          |// Report all deprecated usage within a GraphQL document.
          |export { findDeprecatedUsages } from './findDeprecatedUsages';
          |export type {
          |  IntrospectionDirective,
          |  IntrospectionEnumType,
          |  IntrospectionEnumValue,
          |  IntrospectionField,
          |  IntrospectionInputObjectType,
          |  IntrospectionInputType,
          |  IntrospectionInputTypeRef,
          |  IntrospectionInputValue,
          |  IntrospectionInterfaceType,
          |  IntrospectionListTypeRef,
          |  IntrospectionNamedTypeRef,
          |  IntrospectionNonNullTypeRef,
          |  IntrospectionObjectType,
          |  IntrospectionOptions,
          |  IntrospectionOutputType,
          |  IntrospectionOutputTypeRef,
          |  IntrospectionQuery,
          |  IntrospectionScalarType,
          |  IntrospectionSchema,
          |  IntrospectionType,
          |  IntrospectionTypeRef,
          |  IntrospectionUnionType,
          |} from './getIntrospectionQuery';
          |export { getIntrospectionQuery } from './getIntrospectionQuery';
          |// Gets the target Operation from a Document.
          |export { getOperationAST } from './getOperationAST';
          |// Gets the Type for the target Operation AST.
          |export { getOperationRootType } from './getOperationRootType';
          |// Convert a GraphQLSchema to an IntrospectionQuery.
          |export { introspectionFromSchema } from './introspectionFromSchema';
          |// Sort a GraphQLSchema.
          |export { lexicographicSortSchema } from './lexicographicSortSchema';
          |// Print a GraphQLSchema to GraphQL Schema language.
          |export {
          |  printIntrospectionSchema,
          |  printSchema,
          |  printType,
          |} from './printSchema';
          |// Separates an AST into an AST per Operation.
          |export { separateOperations } from './separateOperations';
          |// Strips characters that are not significant to the validity or execution
          |// of a GraphQL document.
          |export { stripIgnoredCharacters } from './stripIgnoredCharacters';
          |// Comparators for types
          |export {
          |  doTypesOverlap,
          |  isEqualType,
          |  isTypeSubTypeOf,
          |} from './typeComparators';
          |// Create a GraphQLType from a GraphQL language AST.
          |export { typeFromAST } from './typeFromAST';
          |// A helper to use within recursive-descent visitors which need to be aware of
          |// the GraphQL type system.
          |export { TypeInfo, visitWithTypeInfo } from './TypeInfo';
          |// Create a JavaScript value from a GraphQL language AST with a type.
          |export { valueFromAST } from './valueFromAST';
          |// Create a JavaScript value from a GraphQL language AST without a type.
          |export { valueFromASTUntyped } from './valueFromASTUntyped';
        `);
      },
      errors: 1,
    },

All the blank lines are gone (because that’s how the sorting algorithm currently works – all blank lines are removed, because it’s unclear where they should go after sorting).

Need to think about this one.

[remcohaszing]

Perhaps if the export is preceded by a comment, and the comment is preceded by a newline, the preceding newline should be preserved as well.

Alternatively sorting exports could be a separate rule. This would make the feature opt-in, meaning people can choose not to enable it.

[lydell]

I experimented locally and came up with:

1. Exports without from always get a blank line between.
2. Exports with from are surrounded by blank lines if they are multiline. Comments can contribute to being considered as multiline.

Seems to work nicely so far.

[remcohaszing]

Are blank lines surrounding single line exports removed? Considering the following situation:

export {
  QuiteLongVariableName,
  AnotherLongVariableName,
} from './longVariablesModule'

export { whatever } from 'module'

Now one export is removed.

export {
  QuiteLongVariableName,
} from './longVariablesModule'

export { whatever } from 'module'

Apply Prettier:

export { QuiteLongVariableName } from './longVariablesModule'

export { whatever } from 'module'

Now I would expect this plugin to remove the blank line.

export { QuiteLongVariableName } from './longVariablesModule'
export { whatever } from 'module'

[lydell]

Yes, that’s how it works.

Another idea I’m going to explore is sorting chunks imports separately from chunks of re-exports and not touching other exports at all except sorting specifiers in export {a, b}.

[lydell]

I decided to take a break from this during the summer. I'll get back to it this fall.

[lydell]

Closing in favor of #53

@JCrepin Thank you so much for making this PR showing that handling exports is possible! Also a big thanks to both you and @remcohaszing for discussions on how to handle all cases.

One #53 is merged I’m going to fix #50 and #47, then I’ll make a release.