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.

Sign up for GitHub

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

Jump to bottom

Switch to Preconstruct to enable dev bundles #148

Merged
merged 4 commits into from
May 29, 2023
Merged

Switch to Preconstruct to enable dev bundles #148

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b02c307
Switch to Preconstruct to enable dev bundles
Andarist May 15, 2023
82547ef
Fixed the website build
Andarist May 15, 2023
de9ebac
Fixed typecheck
Andarist May 15, 2023
3ba477f
remove redundant part of the config
Andarist May 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions babel.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = {
presets: [require.resolve("@babel/preset-typescript")],
};
21 changes: 19 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{
"name": "react-resizable-panels-repo",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"private": true,
"workspaces": [
"packages/*"
Expand All @@ -10,13 +11,14 @@
"clear:node_modules": "rm -rf ./node_modules && rm -rf ./packages/*/node_modules",
"docs": "cd packages/react-resizable-panels-website && pnpm build",
"lint": "cd packages/react-resizable-panels && pnpm lint",
"prerelease": "rm -rf ./.parcel-cache && cd packages/react-resizable-panels && rm -rf ./dist && pnpm build",
"prerelease": "preconstruct build",
"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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"@types/node": "^18.15.11",
"@types/react": "^18.0.26",
"@types/react-dom": "^18.0.10",
Expand All @@ -32,6 +34,21 @@
"dependencies": {
"@parcel/core": "^2.8.3",
"@parcel/packager-ts": "^2.8.3",
"@parcel/transformer-typescript-types": "^2.8.3"
"@parcel/transformer-typescript-types": "^2.8.3",
"@preconstruct/cli": "^2.6.4"
},
"preconstruct": {
"packages": [
"packages/!(react-resizable-panels-website)"
],
"globals": {
"react": "React"
},
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"exports": {
"importConditionDefaultExport": "default"
},
"___experimentalFlags_WILL_CHANGE_IN_PATCH": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"importsConditions": true
}
}
}
2 changes: 1 addition & 1 deletion packages/react-resizable-panels-website/src/routes/examples/ImperativePanelGroupApi.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Example from "./Example";
import styles from "./ImperativePanelGroupApi.module.css";
import sharedStyles from "./shared.module.css";
import Code from "../../components/Code";
import { ImperativePanelGroupHandle } from "react-resizable-panels/src/PanelGroup";
import { ImperativePanelGroupHandle } from "react-resizable-panels";

type Sizes = {
left: number;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-resizable-panels-website/tests/utils/assert.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ImperativePanelGroupHandle } from "react-resizable-panels";
import { ImperativePanelHandle } from "react-resizable-panels/src";
import { ImperativePanelHandle } from "react-resizable-panels";

export function assert(
expectedCondition: boolean,
Expand Down
30 changes: 26 additions & 4 deletions packages/react-resizable-panels/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,33 @@
"url": "git+https://github.com/bvaughn/react-resizable-panels.git"
},
"source": "src/index.ts",
"main": "dist/react-resizable-panels.js",
"module": "dist/react-resizable-panels.module.js",
"types": "dist/react-resizable-panels.d.ts",
"main": "dist/react-resizable-panels.cjs.js",
"module": "dist/react-resizable-panels.esm.js",
"exports": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

".": {
"types": {
"import": "./dist/react-resizable-panels.cjs.mjs",
"default": "./dist/react-resizable-panels.cjs.js"
},
Comment on lines +16 to +19
Copy link
Owner

@bvaughn bvaughn May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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)

"development": {
"module": "./dist/react-resizable-panels.development.esm.js",
"import": "./dist/react-resizable-panels.development.cjs.mjs",
"default": "./dist/react-resizable-panels.development.cjs.js"
},
"module": "./dist/react-resizable-panels.esm.js",
"import": "./dist/react-resizable-panels.cjs.mjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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%");

Copy link
Owner

Choose a reason for hiding this comment

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

Ooo, neat! Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you push what you have to a branch?

Copy link
Owner

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh! That's...interesting.

Thanks for the help, @Andarist! 🙂

"default": "./dist/react-resizable-panels.cjs.js"
},
"./package.json": "./package.json"
},
"imports": {
"#is-development": {
"development": "./src/env-conditions/development.ts",
"default": "./src/env-conditions/production.ts"
}
},
Comment on lines +31 to +36
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"types": "dist/react-resizable-panels.cjs.d.ts",
"scripts": {
"build": "parcel build",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"lint": "eslint \"src/**/*.{ts,tsx}\"",
"watch": "parcel watch --port=2345"
},
Expand Down
1 change: 1 addition & 0 deletions packages/react-resizable-panels/src/env-conditions/development.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const isDevelopment = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Owner

@bvaughn bvaughn May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

1 change: 1 addition & 0 deletions packages/react-resizable-panels/src/env-conditions/production.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const isDevelopment = false;
Loading