Skip to content

Conversation

@nicknovitski
Copy link
Member

@nicknovitski nicknovitski commented Jun 19, 2025

Why

I noticed that a project with "type": "module" set cannot import anything from @expo/entity's index which uses the export { default as <name> } pattern.

[(node)	] import { Entity, ViewerContext } from '@expo/entity';
[(node)	]          ^
[(node)	] 
[(node)	] SyntaxError: The requested module '@expo/entity' does not provide an export named 'Entity'
[(node)	]     at ModuleJob._instantiate (node:internal/modules/esm/module_job:182:21)
[(node)	]     at async ModuleJob.run (node:internal/modules/esm/module_job:266:5)
[(node)	]     at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:644:26)
[(node)	]     at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5)
[(node)	] 
[(node)	] Node.js v22.15.1

How

I changed the example project to reveal the bug when run, replaced export default with export in all non-test entity files, and regenerated the barrel file.

Test Plan

To see the problem, revert the changes only in packages/entity and run yarn start in packages/entity-example. But notice that no package which has import { foo } from '@expo/entity'; needs to change them.

Copy link
Member Author

nicknovitski commented Jun 19, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codecov
Copy link

codecov bot commented Jun 19, 2025

Codecov Report

Attention: Patch coverage is 95.60117% with 15 lines in your changes missing coverage. Please review.

Project coverage is 97.39%. Comparing base (18a0d29) to head (12836b3).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/entity/src/EntityMutationValidator.ts 0.00% 3 Missing ⚠️
packages/entity/src/IEntityCacheAdapterProvider.ts 0.00% 2 Missing and 1 partial ⚠️
...kages/entity/src/IEntityDatabaseAdapterProvider.ts 0.00% 2 Missing and 1 partial ⚠️
packages/entity/src/GenericSecondaryEntityCache.ts 0.00% 2 Missing ⚠️
...kages/entity/src/errors/EntityCacheAdapterError.ts 0.00% 1 Missing and 1 partial ⚠️
packages/entity/src/IEntityCacheAdapter.ts 0.00% 1 Missing ⚠️
packages/entity/src/IEntityGenericCacher.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #298   +/-   ##
=======================================
  Coverage   97.39%   97.39%           
=======================================
  Files          87       87           
  Lines       11889    11894    +5     
  Branches     1017     1017           
=======================================
+ Hits        11579    11584    +5     
  Misses        303      303           
  Partials        7        7           
Flag Coverage Δ
integration 5.95% <6.45%> (-0.04%) ⬇️
unittest 94.28% <91.49%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nicknovitski nicknovitski changed the base branch from root-scripts to graphite-base/298 June 19, 2025 18:02
@nicknovitski nicknovitski changed the base branch from graphite-base/298 to root-scripts June 19, 2025 18:06
@nicknovitski nicknovitski marked this pull request as ready for review June 19, 2025 18:46
@nicknovitski nicknovitski requested a review from wschurman as a code owner June 19, 2025 18:46
@wschurman wschurman changed the title Fix entity exports for ESM projects feat: Fix entity exports for ESM projects Jun 20, 2025
@wschurman wschurman changed the title feat: Fix entity exports for ESM projects fix: Fix entity exports for ESM projects Jun 20, 2025
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

Definitely cool with this new pattern! Is there a lint rule we can apply to disallow default exports going forward? I think this would be too easy to for contributors to accidentally break it without a lint rule since most people are accustomed to adding default exports.

@nicknovitski nicknovitski force-pushed the root-scripts branch 2 times, most recently from 9270ae3 to 4bdeee5 Compare June 23, 2025 22:44
@nicknovitski nicknovitski force-pushed the root-scripts branch 2 times, most recently from 0351888 to 74553d8 Compare June 23, 2025 22:46
@nicknovitski nicknovitski force-pushed the esm-fix branch 2 times, most recently from aa954cb to 0bf670c Compare June 23, 2025 22:53
@nicknovitski nicknovitski force-pushed the root-scripts branch 2 times, most recently from f00539d to cd9cf6c Compare June 23, 2025 23:10
@nicknovitski nicknovitski changed the base branch from root-scripts to graphite-base/298 June 23, 2025 23:26
@nicknovitski nicknovitski changed the base branch from graphite-base/298 to root-scripts June 23, 2025 23:45
@nicknovitski nicknovitski changed the base branch from root-scripts to graphite-base/298 June 24, 2025 00:04
@nicknovitski nicknovitski changed the base branch from graphite-base/298 to root-scripts June 24, 2025 00:25
@nicknovitski nicknovitski requested a review from wschurman June 24, 2025 16:43
Copy link
Member Author

nicknovitski commented Jun 24, 2025

Merge activity

  • Jun 24, 4:58 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 24, 5:01 PM UTC: Graphite rebased this pull request as part of a merge.
  • Jun 24, 5:03 PM UTC: @nicknovitski merged this pull request with Graphite.

@nicknovitski nicknovitski changed the base branch from root-scripts to graphite-base/298 June 24, 2025 16:58
@nicknovitski nicknovitski changed the base branch from graphite-base/298 to main June 24, 2025 16:59
Before this commit, for projects with `"type": "module"`,  anything
exported from a file `export default` was not resolvable from the barrel
file as a named export.

After this change, the interface of the index is unchanged, but ESM
projects can use it.

To test this, the entity-example package is given `"type": "module"`,
and changed to run with `node --experimental-transform-types`.  Its
imports from other packages do not need to change.  The other
integration tests passing are evidence that commonjs modules also do not
need to change their imports.

This commit also adds an  eslint rule to enforce this standard.
@nicknovitski nicknovitski merged commit 0038d63 into main Jun 24, 2025
3 of 4 checks passed
@nicknovitski nicknovitski deleted the esm-fix branch June 24, 2025 17:03
@nicknovitski nicknovitski mentioned this pull request Jun 25, 2025
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.

3 participants