Skip to content

fix(types): use explicit .js extensions in relative imports - #770

Open
tyler-reitz wants to merge 1 commit into
FirebaseExtended:mainfrom
tyler-reitz:fix/dts-relative-import-extensions
Open

fix(types): use explicit .js extensions in relative imports#770
tyler-reitz wants to merge 1 commit into
FirebaseExtended:mainfrom
tyler-reitz:fix/dts-relative-import-extensions

Conversation

@tyler-reitz

Copy link
Copy Markdown
Contributor

Summary

Fixes #769. Adds explicit .js extensions to relative import specifiers in src, so the emitted .d.ts stop carrying extensionless paths that node16 / nodenext resolution rejects.

Source-only change, 10 files, all mechanical. moduleResolution: bundler accepts both forms, so nothing changes for the build or for bundler consumers.

The bug

TypeScript emits relative specifiers unchanged, so export * from './auth' in src/index.ts becomes from './auth' in dist/index.d.ts. Under node16 that is not a valid specifier, and a consumer on "moduleResolution": "node16" gets resolution errors reading our declarations.

Two shapes were involved:

  • 16 extensionless relative imports across 10 emitted .d.ts files (from './useObservable')
  • 6 bare directory specifiers, from './' and from '.', in auth, database, firestore, functions, remote-config, sdk, storage and useObservable. These resolve to the barrel and now point at ./index.js explicitly

The runtime bundles resolve fine, which is why nothing in the build caught it.

Verification

@arethetypeswrong/cli against the packed build, with no ignore rules:

before after
node10 pass pass
node16 (from CJS) no types no types
node16 (from ESM) internal resolution error pass
bundler pass pass

The remaining node16 (from CJS) failure is a different bug: no types condition on either exports branch, fixed in #766. Applying both, all four modes pass with no suppression, which is the acceptance test #769 asks for.

Also confirmed: tsc --noEmit clean on both tsconfig.json and tsconfig.test.json, vite build succeeds, and zero extensionless relative specifiers remain in the emitted declarations.

Relationship to #766

#766 adds attw to CI and currently suppresses this via .attw.json:

{ "ignoreRules": ["internal-resolution-error"] }

That ignore is rule-global, so it hides any future error of the class. If this lands first, #766 can drop the ignore before it merges and the suppression never ships. If #766 lands first, deleting the entry becomes this issue's acceptance test instead. Either order works; this one is tidier.

Test plan

  • npm test
  • npx tsc --noEmit on both tsconfigs
  • attw on the packed build shows no internal resolution error

The emitted .d.ts inherited source's extensionless relative specifiers
(`from './useObservable'`, and `from '.'` in two files), which node16 and
nodenext resolution reject. Consumers on those settings hit resolution
errors reading our declarations, while the runtime bundles were fine, so
nothing in the build surfaced it.

`moduleResolution: bundler` accepts both forms in source, so this is a
no-op for the build and for bundler consumers. Verified with
@arethetypeswrong/cli against the packed build, with no ignore rules:

  before   node16 (from ESM)  internal resolution error
  after    node16 (from ESM)  pass

node16 (from CJS) still reports "no types" on this branch; that is the
missing `types` export condition, fixed separately in FirebaseExtended#766. With both
changes applied, all four resolution modes pass with no suppression.

Fixes FirebaseExtended#769.
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@armando-navarro armando-navarro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks Tyler, and thanks for filing #769 out of the #766 discussion. This is the fix I was hoping for, and it approves cleanly.

What I verified

I reproduced the problem on main and confirmed the fix against a fresh build, using attw with no ignore rules:

  • On main, node16-from-ESM resolves the declarations to an internal resolution error, and the emitted .d.ts carry 16 extensionless relative specifiers.
  • On this branch, node16-from-ESM goes green and zero extensionless relative specifiers remain in the emitted declarations.
  • node16-from-CJS stays on "no types", which is the missing types condition and lands with #766, exactly as your description says.
  • tsc --noEmit is clean on both tsconfig.json and tsconfig.test.json, and vite build succeeds.
  • The runtime bundles (dist/index.js, dist/index.umd.cjs) carry no relative specifiers, so this is resolved away at build time and nothing changes for runtime or for bundler consumers.

I also reverted a single specifier back to extensionless as a check, and the internal resolution error came straight back, so the fix is doing real work across all 29 edits, not riding on one.

One thing worth a follow-up (non-blocking)

The fix is correct, but I do not think it is self-enforcing yet. tsconfig.json stays on moduleResolution: bundler, which accepts both the extensionless and the .js forms, so tsc will stay green if an extensionless relative import creeps back in later. And there is no attw/publint job in CI today, so nothing catches a regression until #766 adds one.

Your own note on #769 floated moving moduleResolution in the same change so the compiler enforces this. Would you want to do that here, or add a lint rule for relative-import extensions, so this cannot silently regress before #766 lands? I have not checked whether switching resolution builds cleanly (it can have knock-on effects), so I am asking rather than recommending.

On the #766 suppression

Confirming the coordination in your description: with this branch plus #766's types condition, all four attw modes pass with no ignore rules. So whichever order these land, #766 can drop the .attw.json ignoreRules entry and the suppression never ships permanently. Landing this one first is the tidier path, as you say.

Small note

src/functions.tsx now has two separate imports that both read from './index.js' (they were from './' and from '.' before). Totally valid, just foldable into one line if you feel like it.

If any of this reads wrong, say so and I will take another look.

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.

types: emitted .d.ts use extensionless relative imports, which node16 resolution rejects

2 participants