Switch to Preconstruct to enable dev bundles #148

Andarist · 2023-05-15T14:49:13Z

No description provided.

vercel · 2023-05-15T14:49:17Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-resizable-panels	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 16, 2023 7:14am

Andarist · 2023-05-15T14:51:14Z

package.json

@@ -1,4 +1,5 @@
 {
+  "name": "react-resizable-panels-repo",


it was a Preconstruct requirement, we actually should lift this off - I don't see a strong reason why this would be hardly required for the monorepo root

Andarist · 2023-05-15T14:52:59Z

package.json

    "prettier": "prettier --write \"**/*.{css,html,js,json,jsx,ts,tsx}\"",
    "prettier:ci": "prettier --check \"**/*.{css,html,js,json,jsx,ts,tsx}\"",
    "typescript": "tsc --noEmit",
    "typescript:watch": "tsc --noEmit --watch"
  },
  "devDependencies": {
+    "@babel/preset-typescript": "^7.21.5",


Preconstruct is Babel-based so u get a good control over the output (plugins support and all). I don't think that u need this anyhow but well, I had to opt into TypeScript syntax somehow. We plan to explore supporting alternative transpilers but, quite frankly, we run Babel in a separate thread and stuff and it's really not that bad perf-wise - especially for just a few files. I use this in a couple of projects and never noticed any noticeable slowdowns

Andarist · 2023-05-15T14:54:29Z

packages/react-resizable-panels/package.json

  "scripts": {
-    "build": "parcel build",


it's already executed by the root so there is no strong need to have it here - atm Preconstruct doesn't exactly allow per-directory builds, it's running through the whole monorepo cause we need to sort builds in a topological order to generate TS declarations. I have an idea how to lift this requirement though and plan to work on this.

If needed this script could just proxy to the top-level build script

Andarist · 2023-05-15T15:37:43Z

packages/react-resizable-panels/package.json

+  "imports": {
+    "#is-development": {
+      "development": "./src/env-conditions/development.ts",
+      "default": "./src/env-conditions/production.ts"
+    }
+  },


You can freely reorganize this, only keys (aka conditions) are relevant (and the fact that they are pointing to files in src). I'm just experimenting with how to best organize those files - their implementation can also be just anything but we use simple named exports with boolean values in them here. The recommendation is for all files used for a certain imports to have the same module interface (same types) - otherwise it could lead to weird problems, remember that they should be "swappable".

Those are only used in development (when using Preconstruct, since those are valid things in node etc and u could leave imports to them as-is). preconstruct build inlines/bundles those files so all of that should still work with older runtime/bundlers/etc

Andarist · 2023-05-15T15:38:54Z

package.json

+    "exports": {
+      "importConditionDefaultExport": "default"
+    },
+    "___experimentalFlags_WILL_CHANGE_IN_PATCH": {


the name is scary since we just shipped this as a new feature but we don't intend to break this, if we promote this to a stable feature, rename this or anything then preconstruct fix will just become capable of migrating this to the new format

Andarist · 2023-05-15T15:42:59Z

packages/react-resizable-panels/package.json

-  "types": "dist/react-resizable-panels.d.ts",
+  "main": "dist/react-resizable-panels.cjs.js",
+  "module": "dist/react-resizable-panels.esm.js",
+  "exports": {


All of this is a little bit crazy... but well, it's what we found to cover all of the "compatibility boxes". This should work in old nodes, new nodes, with require, with import, with different bundlers, with different versions of TS and its moduleResolution setting etc

For example, previously your package was CJS-only and thus in pure-ESM environment it was consumable through a default import (as well as through named ones):
TS playground.

This is now fixed - no default export is provided and one can't use a default import to consume this module (that matches how you defined its interface)

Andarist · 2023-05-15T15:47:45Z

packages/react-resizable-panels/package.json

+        "default": "./dist/react-resizable-panels.development.cjs.js"
+      },
+      "module": "./dist/react-resizable-panels.esm.js",
+      "import": "./dist/react-resizable-panels.cjs.mjs",


.cjs.mjs might be a little bit confusing here - why do we point to something with .cjs in the filename from within the import condition?

we avoid the dual package hazard by using the es module wrapper technique. This way only one copy of your library (given the matching versions etc) ends up being loaded and it's always a CJS version (either directly when loaded using require or through the ESM wrapper if loaded using import).

Isn't this bad for bundlers? well, not quite - because we also use the module condition that points to the ESM bundles. This is also understood by all modern bundlers and it's equivalent of the old package.json#module. Both require and import are capable of resolving using this condition in bundlers and thanks to that you will also only ever load one instance of this library.

Hey @Andarist! Looking at this tonight, to add some dev-only warnings, and wondering how I'm supposed to use this new condition. Referencing isDevelopment in TypeScript doesn't seem possible without directly importing one of the two files, which would seem to defeat the purpose. I'm probably overlooking something silly.

This should work:

import { isDevelopment } from "#is-development"

if it doesnt - could u push out ur attempt to use it so i could take a look at the exact situation?

I confirmed that with this patch:

diff --git a/packages/react-resizable-panels/src/PanelGroup.ts b/packages/react-resizable-panels/src/PanelGroup.ts index 6beadfc..0c73f4f 100644 --- a/packages/react-resizable-panels/src/PanelGroup.ts +++ b/packages/react-resizable-panels/src/PanelGroup.ts @@ -46,6 +46,7 @@ import { } from "./utils/group"; import { loadPanelLayout, savePanelGroupLayout } from "./utils/serialization"; import { isServerRendering } from "./utils/ssr"; +import { isDevelopment } from "#is-development"; const debounceMap: { [key: string]: ( @@ -196,7 +197,9 @@ function PanelGroupWithForwardedRef({ 0 ); - assert(total === 100, "Panel sizes must add up to 100%"); + if (isDevelopment) { + assert(total === 100, "Panel sizes must add up to 100%"); + } setSizes(sizes); },

I can only found the wrapped code in .development. builds:

> grep -R 'must add up to' packages/react-resizable-panels/dist packages/react-resizable-panels/dist/react-resizable-panels.development.esm.js: assert(total === 100, "Panel sizes must add up to 100%"); packages/react-resizable-panels/dist/react-resizable-panels.development.cjs.js: assert(total === 100, "Panel sizes must add up to 100%");

Ooo, neat! Thank you

Could you push what you have to a branch?

Away from the desk for a bit but the same build error on the suspense package PR we’re talking on so that’s probably an easier starting place :)

Hm, Parcel definitely has support for every puzzle piece here - I'm not yet sure what's wrong and since Parcel's internals are written partially in Rust I can't quite debug this in depth :S

I'll continue digging into this - there is definitely at least one Parcel bug here to be found because it crashes on existing but empty diagnostics internally.

If I don't come up with a fix soon-ish I will propose some alternative solution to the problem here. I think that preconstruct dev might be a substitute for package.json#source but I have to test it out with Parcel.

Just add this to your root package.json:

"@parcel/resolver-default": { "packageExports": true }

It seems that Parcel introduced exports support as opt-in (weird choice if you ask me) and that impacts imports as well. The DX here could definitely be improved on the Parcel's side of things - at the very least they could inform the user what went wrong as the situation itself is pretty clear here. Importing a module specifier with a leading hash isn't really ambiguous as far as I know.

Oh! That's...interesting.

Thanks for the help, @Andarist! 🙂

Andarist · 2023-05-15T15:49:37Z

packages/react-resizable-panels/src/env-conditions/development.ts

@@ -0,0 +1 @@
+export const isDevelopment = true;


There is a small caveat right now that you won't get auto-imports for this #is-development thing. There is a tracking issue about this here. Emma (Preconstruct's author) is already working on adding support for that - that will benefit everybody since package.json#imports are by no means Preconstruct's feature.

Am I understanding you correctly to mean that e.g. VS Code auto-imports will stop working for the monorepo?

I think you're just saying that attempts to import isDevelopment will need to be manually written?

I think you're just saying that attempts to import isDevelopment will need to be manually written?

This. Once you add the import to the file then isDevelopment will appear in the suggestions with it - it's just about the auto-import thing (and it's a temporary situation, hopefully, since the support in the TS lang server for this will come in the future).

bvaughn · 2023-05-15T21:40:17Z

packages/react-resizable-panels/package.json

+      "types": {
+        "import": "./dist/react-resizable-panels.cjs.mjs",
+        "default": "./dist/react-resizable-panels.cjs.js"
+      },


I don't think I understand why this "types" key exists or what it means (vs the higher level "types" key)

package.json#exports define everything about the package (well, almost, but let's simplify here) that is. It's like a higher-priority manifest describing the content of the package.

Consider this:

{ "name": "my-lib", "main": "./lib/main.js", "exports": "./lib/public.js" }

In here, package.json#main is completely ignored. package.json#exports win and that's all. This should be the behavior of every tool that understands exports. As soon as a package adds that it opts into that new behavior - other keys might only be used as "fallback" for tools from the pre-exports era.

However, TS has chosen a different route - likely because following this strictly would be quite disruptive for the ecosystem. So despite the fact that modern TS versions know what to do with exports... they ignore it by default. Most projects are configured with moduleResolution: node and that ignores package.json#exports completely.

But as soon as you opt in to the new semantics with moduleResolution: node16, they start matching the "runtime semantics" - that is: if a package has exports then other fields are ignored. And yes, that includes the top-level package.json#types.

Note that types in exports are not mandatory (just like package.json#types isn't). You can alternatively rely on the "sibling lookup" (TS always will try to load file.d.ts when importing~ file.js, file.d.mts when importing file.mjs, etc). Personally, I find the explicit types field (within exports or the top-level one) to be more explicit and easier to reason about.

Just want to add on to @Andarist's explanation, the types condition is here specifically so that the types correctly resolve regardless of the conditions specified in https://www.typescriptlang.org/tsconfig#customConditions (currently it would technically work without the types condition anyway because of a TypeScript bug but that shouldn't be relied on)

bvaughn · 2023-05-15T21:42:46Z

I also get a failure when I try to build the website locally (Node v18.12.1 if relvant):

p install
p run docs

🚨 Build failed.

Error: Failed to install @babel/core: pnpm failed to install modules

  Error: Failed to install @babel/core: pnpm failed to install modules
      at $32ea97b83cf5d752$var$install 
  (/Users/bvaughn/Documents/git/oss/react-resizable-panels/node_modules/.pnpm/@parcel+package-manager@2.8.3_@parcel+core@2.8.3/node_modules/@parcel/package-manager/lib/index.js:3690:15)
      at async $b0fd219fea43bcac$export$2e2bcd8739ae039._runFn 
  (/Users/bvaughn/Documents/git/oss/react-resizable-panels/node_modules/.pnpm/@parcel+utils@2.8.3/node_modules/@parcel/utils/lib/index.js:33579:13)
      at async $b0fd219fea43bcac$export$2e2bcd8739ae039._next 
  (/Users/bvaughn/Documents/git/oss/react-resizable-panels/node_modules/.pnpm/@parcel+utils@2.8.3/node_modules/@parcel/utils/lib/index.js:33572:9)

 ELIFECYCLE  Command failed with exit code 1.
 ELIFECYCLE  Command failed with exit code 1.

Andarist · 2023-05-15T22:22:57Z

@bvaughn fixed that, Parcel has seen the added babel.config.js and assumed that I wanted to use Babel with Parcel. I successfully built the website locally and tested the watch command as well.

Andarist · 2023-05-15T22:53:38Z

packages/react-resizable-panels/src/index.ts


 export {
  // TypeScript types
  ImperativePanelGroupHandle,
  ImperativePanelHandle,
  Panel,
+  PanelOnCollapse,


Those added things were not exported but yet they were consumable... I have no idea how that worked. If we inspect the content od the current declaration files we can clearly see that the types that I had to export from here were not exported:
https://unpkg.com/browse/react-resizable-panels@0.0.45/dist/react-resizable-panels.d.ts

I found out why this worked before (with @emmatown's help) and opened a PR in Parcel with a change that would help to avoid accidental leakage like this: parcel-bundler/parcel#9016

Andarist · 2023-05-15T22:54:57Z

packages/react-resizable-panels-website/tests/CursorStyle.spec.ts

@@ -1,12 +1,36 @@
 import { expect, Page, test } from "@playwright/test";
 import { createElement } from "react";
 import { Panel, PanelGroup, PanelResizeHandle } from "react-resizable-panels";
-import { getCursorStyle } from "react-resizable-panels/src/utils/cursor";


This was reaching for a private~ util (and with exports we can't reach into undeclared files, although we could add src/* "forwarding", I just wanted to avoid this). I decided to inline it in this test file - if you have any other preferred about to solve this, let me know.

emmatown · 2023-05-16T00:13:08Z

package.json

+    "globals": {
+      "react": "React"
+    },


Did you mean to add this @Andarist? I can't see any UMD builds (and I'd personally recommend not adding them)

No, I wasn't sure if UMDs were opt-in with Preconstruct so I included this and forgot to revisit it. Thanks for catching this! I removed it now.

bvaughn · 2023-05-28T15:26:29Z

Looking at this again this morning.

Props	Cons
Separate dev and production bundles would let me add dev-only warnings which is really nice The Preconstruct bundle size is ~14% smaller I can remove the component `displayName` hack since Preconstruct doesn't mangle the names like Parcel does	More complex project infra (Parcel for one thing, Preconstruct+Babel for another) Dev work flow (`pnpm watch`) uses ParcelJS but production bundle (`pnpm prerelease`) uses Presconstruct; that means I'd be testing different things when iterating vs releasing Package exports syntax is unfamiliar to me Don't love the code duplication in `CursorStyle.spec.ts` but I can address that separately

I think overall this looks like a change worth merging. I just want to give it a little more testing this weekend.

Andarist · 2023-05-28T19:30:43Z

Dev work flow (pnpm watch) uses ParcelJS but production bundle (pnpm prerelease) uses Presconstruct; that means I'd be testing different things when iterating vs releasing

This one could be addressed by running Preconstruct in watch mode so Parcel would see the dist files. This is a little bit less convenient than just loading src for your package instead of dist files but well, tradeoffs - it depends on what you care most here. This is quite a popular problem~ and can be observed a lot in the ecosystem, even among your own packages :P For example, in react-error-boundary you are running tests through Jest and ts-jest so you are "testing different things when iterating vs releasing" there 😉

bvaughn · 2023-05-28T19:55:29Z

Yeah, that's fair.

I think it's nice to test as close to the thing that I'm releasing as possible, to reduce the chance that I overlook something critical (and reduce the burden of fixing a bug and publishing a release for a package I don't use often– context switching is hard and this is all extra curricular work)

Andarist · 2023-05-28T20:47:15Z

Agreed that it's nice - so if that's crucial for you then I'd say that the only way to solve this here is to run 2 watchers in parallel: 1 for the lib, 1 for the app/docs.

bvaughn · 2023-05-29T01:52:25Z

Agreed that it's nice - so if that's crucial for you then I'd say that the only way to solve this here is to run 2 watchers in parallel: 1 for the lib, 1 for the app/docs.

Right. That's what I was doing before :)

I do often iterate on the component library using the docs site as a testing ground, so it is pretty important to my work flow that I run both in watch flow. Whether it's important that the library is built by Preconstruct vs Parcel when iterating is a less distinction important but would be nice.

Andarist · 2023-05-29T12:36:00Z

As discussed on Twitter - I think that it's likely that Preconstruct could output source maps for you. From what I can tell, it's just a matter of passing a boolean to a couple of functions (I have this change setting locally in a draft on my machine). The only question is - if you'd even want to enable them with the pretty readable Preconstruct's output.

It seems that some tools just don't load source maps from node_modules anyway - but that's kind on them. To quote Emma:

my understanding is that most build tools don't actually usually read source maps of dependencies or sometimes they sort of read them and things break or the browser gets confused because there's a source map comment but no source map so it's easier to just not have source maps since the output is probably readable enough

I think though that if a tool trips somehow on a source map that is available in the module then it's on that tool and that it should fix its own issue. So if the package author's desire is to ship source maps, I think that Preconstruct should allow them.

bvaughn · 2023-05-29T20:38:32Z

The only question is - if you'd even want to enable them with the pretty readable Preconstruct's output.

I'd probably prefer them, but I don't view them as necessary– given how readable Preconstruct code is.

So if the package author's desire is to ship source maps, I think that Preconstruct should allow them.

Agreed with this, generally. I don't think they're a blocker for me though.

bvaughn · 2023-05-29T20:45:18Z

Thank you! 🙇🏼

bvaughn · 2023-05-29T21:03:26Z

Published in 0.0.48

vercel bot had a problem deploying to Preview May 15, 2023 14:49 Failure

Andarist commented May 15, 2023

View reviewed changes

Andarist force-pushed the enable-dev-prod-bundles branch from 9e0b476 to 7093e20 Compare May 15, 2023 15:36

vercel bot had a problem deploying to Preview May 15, 2023 15:37 Failure

Andarist commented May 15, 2023

View reviewed changes

Switch to Preconstruct to enable dev bundles

b02c307

Andarist force-pushed the enable-dev-prod-bundles branch from 7093e20 to b02c307 Compare May 15, 2023 16:00

vercel bot had a problem deploying to Preview May 15, 2023 16:00 Failure

bvaughn reviewed May 15, 2023

View reviewed changes

Fixed the website build

82547ef

vercel bot deployed to Preview May 15, 2023 22:22 View deployment

Fixed typecheck

de9ebac

vercel bot deployed to Preview May 15, 2023 22:52 View deployment

Andarist commented May 15, 2023

View reviewed changes

emmatown reviewed May 16, 2023

View reviewed changes

remove redundant part of the config

3ba477f

vercel bot deployed to Preview May 16, 2023 07:14 View deployment

Andarist mentioned this pull request May 16, 2023

Avoid leaking local types by the generated declarations parcel-bundler/parcel#9016

Open

2 tasks

Andarist marked this pull request as ready for review May 16, 2023 07:44

bvaughn mentioned this pull request May 28, 2023

Switch to Preconstruct bvaughn/react-error-boundary#144

Merged

bvaughn merged commit 0140895 into bvaughn:main May 29, 2023

Andarist deleted the enable-dev-prod-bundles branch May 29, 2023 21:38

Switch to Preconstruct to enable dev bundles #148

Switch to Preconstruct to enable dev bundles #148

Conversation

Andarist commented May 15, 2023

vercel bot commented May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented May 15, 2023 • edited Loading

Andarist commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented May 28, 2023 • edited Loading

Andarist commented May 28, 2023

bvaughn commented May 28, 2023

Andarist commented May 28, 2023

bvaughn commented May 29, 2023 • edited Loading

Andarist commented May 29, 2023

bvaughn commented May 29, 2023

bvaughn commented May 29, 2023

bvaughn commented May 29, 2023

vercel bot commented May 15, 2023 •

edited

Loading

bvaughn May 15, 2023 •

edited

Loading

bvaughn May 15, 2023 •

edited

Loading

bvaughn commented May 15, 2023 •

edited

Loading

bvaughn commented May 28, 2023 •

edited

Loading

bvaughn commented May 29, 2023 •

edited

Loading