feat: bundle packages to be Ecmascript modules (T-186) #64

desfero · 2023-01-05T15:35:50Z

The current support of ESM modules seems to be inconsistent and some of the libraries still don’t provide proper support.
All major bundlers support ESM but also on top of that provide good patching when a module is not proper ESM modules.
The most strict implementation has the Node.js which is the cause reason why next.js example fails.
(For .e.g when no package.json#exports is present it will switch back and use package.json#module which is the case with apollo client (apollographql/apollo-feature-requests#287 and apollographql/apollo-client#9890) but node.js doesn't support package.json#modules and therefore converts module to CommonJS which is different than what bundlers doo (see https://nodejs.org/api/esm.html#commonjs-namespaces))

I had also a look if we can get rid of folder packages (the one with just package.json files) but unfortunately, we have at least 2 blockers:

Typescript won’t use package.json#exports until we switch our packages to use moduleResolution: node nextwhich on the other hand would require us to add.js` to all relative imports (see types field in package.json exports doesn't work microsoft/TypeScript#51862 (comment))
Metro is not supporting package.json#exports (see Feature: support new package.json exports field facebook/metro#670)

I tried to get rid also of mocks folders as they are importing devDependencies which is something tsup does bundle by default (and we want to still keep import and use the single version of jest)
I have considered using project references or importing directly src files but without any quick success:

Project references are not working good with ts-jest (see Tests fail when using TypeScript project references kulshekhar/ts-jest#1648)
Direct src imports won’t work cause ts-jest is using package.json.exports where mocks that point directly to src should not be present as we omit src folder when publishing package
So for now we just make sure tsup doesn't bundle devDependencies by overriding external config.

The good news is that we can drop CommonJS support as all bundlers seem to properly handle ES imports/exports.

There is a quirk with jest as it only supports imports/exports when running in ESM modules mode (https://jestjs.io/docs/ecmascript-modules). We can’t enable ESM for jest until apollo fixes Types are broken with compilerOptions.module set to nodenext due to imports missing file extensions apollographql/apollo-client#9890. So for now I’m just transpiling @lens-protocol packages by using transformIgnorePatterns in jest.config.js.

So to summarise:

We only now ship ESM Modules
Until the @apollo/client fixes the ESM modules support Next.js example won’t work without either
- transpiling @lens-protocol package (the easiest and recommended)
- making sure that any of the @lens-protocol imports don’t get imported by node.js (which basically means no @lens-protocol imports inside files that are imported during SSR

height · 2023-01-05T15:35:52Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

desfero · 2023-01-06T11:13:56Z

Link T-186

desfero · 2023-01-06T11:16:33Z

examples/web-wagmi/src/profiles/components/ProfileCard.tsx

-      <Link to={`/profile/handle/${profile.handle}`}>
-        <ProfilePicture picture={profile.picture} />
-      </Link>
+      <ProfilePicture picture={profile.picture} />


There is no support, for now, to provide route param with the handle so it was pointing to a nonexisting route.

cesarenaldi · 2023-01-09T13:17:29Z

Thank you for the exhaustive explanation! I'll provide a proper code review shortly.

desfero · 2023-01-09T13:30:04Z

There is one thing I forgot to include, the generation of d.ts.map files, which means DX gonna be bad as typescript won't nicely navigate to implementation between packages. Let's review that PR and will do the followup with DX improvements this week.

cesarenaldi · 2023-01-10T10:28:06Z

examples/web-wagmi/tsconfig.json

@@ -3,7 +3,7 @@
    "target": "ESNext",
    "useDefineForClassFields": true,
    "lib": ["DOM", "DOM.Iterable", "ESNext"],
-    "allowJs": false,
+    "allowJs": true,


Is this intentional?

All changes to the package.json in src/packages are intentional, this one is just making everything in sync across all tsconfig.

cesarenaldi · 2023-01-10T10:39:23Z

Not a blocker.

Do we still need the separation between tsconfig.json and tsconfig.base.json in each (most) of the packages?

Or is that left as precursor for a packages/tsconfig shared TS config package?

desfero · 2023-01-10T10:52:02Z

@cesarenaldi the tsconfig.base.json is left intentionally until we get a better understanding of how to solve local development issues that we have right now (a need to rebuild every time, bad auto imports).

cesarenaldi · 2023-01-10T10:53:11Z

packages/shared-kernel/jest.config.js

  testEnvironment: 'node',
  testPathIgnorePatterns: ['/node_modules/', '/dist/'],
+  transformIgnorePatterns: [`/node_modules/(?!@lens-protocol/*)`],


I assume this was previously working cause it was loading the CJS files.

Jest support for ESM confuses me all the times I look into it. What would happen if we were to keep ts-jest preset and add NODE_OPTIONS=--experimental-vm-modules to the Jest script?

cesarenaldi · 2023-01-10T10:55:28Z

packages/react/src/transactions/adapters/PendingTransactionGateway/PendingTransactionGateway.ts

@@ -12,7 +12,7 @@ import {
 } from '@lens-protocol/domain/use-cases/transactions';
 import { assertNever, invariant } from '@lens-protocol/shared-kernel';
 import { IStorage } from '@lens-protocol/storage';
-import differenceBy from 'lodash/differenceBy';
+import differenceBy from 'lodash/differenceBy.js';


Out of curiosity, are you aware of any plan for Lodash to have proper exports configuration?

There is lodash-es that's under the hood using es exports, maybe it's worth to migrate at some point (or completely remove the use of lodash given it's used in just a couple of places)

cesarenaldi · 2023-01-10T10:56:56Z

Great work! Thank you for looking into this annoying issue.

# Conflicts: # examples/web-wagmi/src/publications/components/PublicationCard.tsx

Bundle packages

6494f96

desfero self-assigned this Jan 5, 2023

Remove commonjs bundle format

4cf390b

desfero changed the title ~~feat: bundle packages to be Ecmascript modules compliant~~ feat: bundle packages to be Ecmascript modules Jan 6, 2023

pawellula added 3 commits January 6, 2023 10:46

Merge branch 'main' into feat/T-186_Support_Next.js

62d38ed

Fix broken deep imports

c325e71

Fix broken lints in example app

8fd1dad

desfero force-pushed the feat/T-186_Support_Next.js branch from d8007d4 to 8fd1dad Compare January 6, 2023 10:41

Add transpilePackages flag to next.js example

393d2fe

desfero changed the title ~~feat: bundle packages to be Ecmascript modules~~ feat: bundle packages to be Ecmascript modules (T-186) Jan 6, 2023

desfero marked this pull request as ready for review January 6, 2023 11:13

desfero commented Jan 6, 2023

View reviewed changes

cesarenaldi reviewed Jan 10, 2023

View reviewed changes

cesarenaldi approved these changes Jan 10, 2023

View reviewed changes

pawellula added 2 commits January 12, 2023 11:09

Merge branch 'main' into feat/T-186_Support_Next.js

746db7a

# Conflicts: # examples/web-wagmi/src/publications/components/PublicationCard.tsx

Generate typescript d.ts.map files for better local DX

57899f5

cesarenaldi approved these changes Jan 12, 2023

View reviewed changes

desfero merged commit cc453cc into main Jan 12, 2023

desfero deleted the feat/T-186_Support_Next.js branch January 12, 2023 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bundle packages to be Ecmascript modules (T-186) #64

feat: bundle packages to be Ecmascript modules (T-186) #64

desfero commented Jan 5, 2023 •

edited

Loading

height bot commented Jan 5, 2023

desfero commented Jan 6, 2023

desfero Jan 6, 2023

cesarenaldi commented Jan 9, 2023

desfero commented Jan 9, 2023

cesarenaldi Jan 10, 2023

desfero Jan 12, 2023

cesarenaldi commented Jan 10, 2023

desfero commented Jan 10, 2023 •

edited

Loading

cesarenaldi Jan 10, 2023

cesarenaldi Jan 10, 2023

desfero Jan 12, 2023

cesarenaldi commented Jan 10, 2023

feat: bundle packages to be Ecmascript modules (T-186) #64

feat: bundle packages to be Ecmascript modules (T-186) #64

Conversation

desfero commented Jan 5, 2023 • edited Loading

height bot commented Jan 5, 2023

desfero commented Jan 6, 2023

desfero Jan 6, 2023

Choose a reason for hiding this comment

cesarenaldi commented Jan 9, 2023

desfero commented Jan 9, 2023

cesarenaldi Jan 10, 2023

Choose a reason for hiding this comment

desfero Jan 12, 2023

Choose a reason for hiding this comment

cesarenaldi commented Jan 10, 2023

desfero commented Jan 10, 2023 • edited Loading

cesarenaldi Jan 10, 2023

Choose a reason for hiding this comment

cesarenaldi Jan 10, 2023

Choose a reason for hiding this comment

desfero Jan 12, 2023

Choose a reason for hiding this comment

cesarenaldi commented Jan 10, 2023

desfero commented Jan 5, 2023 •

edited

Loading

desfero commented Jan 10, 2023 •

edited

Loading