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

Enforce file extension in import #380

Merged
merged 3 commits into from
Jun 7, 2022
Merged

Conversation

calebeby
Copy link
Member

@calebeby calebeby commented Jun 6, 2022

From the changeset:

Enabled @cloudfour/n/file-extension-in-import.

require('./foo') → ✅ require('./foo.js')
import * as foo from './foo' → ✅ import * as foo from './foo.js'

If the file that you are importing is a .ts file, you must import it as .js, because of a decision that the TypeScript team made.

It is auto-fixable.

History of extension-less imports:

  • The Node Resolution Algorithm for CommonJS has a feature where when you require() paths it will look in several places for the module, until it finds one:
    ex: require('./foo') checks for:
    • ./foo
    • ./foo.js
    • ./foo.json
    • ./foo.node
    • ./foo/package.json with main field
    • ./foo/index.js
  • People began implementing require() in browsers with bundlers (Browserify and then Webpack)
  • ES6 Imports began to be proposed as a part of the JS language
  • Bundlers (Browserify/Webpack) began supporting import/export, by using the same internal mechanisms as require(), meaning that they preserved the resolution algorithm used by require().
  • The ES Imports spec treats import paths as URL's (e.g. import './foo' resolves like <a href="./foo"). There is no mechanism for rewriting paths like ./foo -> ./foo.js (except if the server provides redirects)
  • This means that the bundler's implementation of ES modules doesn't match the spec because they do rewrite imports.
  • Other bundlers and tools (parcel, esbuild, rollup-plugin-node-resolve, typescript) followed what webpack and browserify did, attempting to minimize compatibility issues when switching between bundlers.
  • Browsers began implementing ES modules natively. According to the spec, there is no path rewriting.
  • Node began implementing ES modules natively, following the spec. There is no path rewriting. This means that Node has a different resolution algorithm for require than import.
  • TypeScript added support for opting-in to using Node's modern resolution with import statements by setting moduleResolution to node12. When that option is set, TypeScript will no longer resolve import './foo' to ./foo.js or ./foo.ts, and it will throw an error (same as Node). The TS team decided that in order to import a file called foo.ts, you must use a .js import: import './foo.js', even though the file foo.js does not exist. TS will not let you use import './foo.ts', even though ./foo.ts exists. The rationale is that if TS was to rewrite the import path during code emit from .ts to .js, it would be a functionality change to the JS code, which they avoid with a ten-foot pole. (I don't think this decision was the right one, but it is what it is).

In other words, syntax like import './foo' is syntax that was never really valid/specified. It is a mishmash of the ES import syntax and the CommonJS module specifier syntax. require('./foo') is valid and import './foo.js' is valid.

To help move our projects towards supporting native-node ESM and native-browser ESM, this PR enforces the use of file extensions in all path imports. That means it will switch require('./foo') to require('./foo.js') even though that change is not needed. The goal is consistency across all imports/requires, even in projects that use CommonJS or a bundler that shims in CommonJS resolution into ESM.

@changeset-bot
Copy link

changeset-bot bot commented Jun 6, 2022

🦋 Changeset detected

Latest commit: 416ed42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudfour/eslint-plugin Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines 1 to +3
import { getNextWrappingIndex, getNextNonDisabledIndex } from '../../utils';
import { getHighlightedIndexOnOpen, getDefaultValue } from '../utils';
import { getItemIndexByCharacterKey } from './utils';
import { getHighlightedIndexOnOpen, getDefaultValue } from '../utils.js';
import { getItemIndexByCharacterKey } from './utils.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why does the following not have the .js extension?

import { getNextWrappingIndex, getNextNonDisabledIndex } from '../../utils';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it only enforces it for files which it can find/actually exist.

So in this case, the file doesn't exist (since this file is just one of the fixtures files picked out of the downshift project, and other files in that project weren't included).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting.

@calebeby calebeby merged commit 80af81b into main Jun 7, 2022
@calebeby calebeby deleted the n/file-extension-in-import branch June 7, 2022 23:23
@github-actions github-actions bot mentioned this pull request Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants