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

fix(remix-dev): yarn pnp compatibility plugin #3594

Closed
wants to merge 3 commits into from
Closed

fix(remix-dev): yarn pnp compatibility plugin #3594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd11f38
fix: plugins
lensbart Jun 28, 2022
053fd01
chore: sign CLA
lensbart Jun 24, 2022
73ef80c
fix: yarn config get
lensbart Jun 28, 2022
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
1 change: 1 addition & 0 deletions contributors.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@
- lachlanjc
- laughnan
- lawrencecchen
- lensbart
- leo
- leon
- levippaul
Expand Down
12 changes: 7 additions & 5 deletions packages/remix-dev/cli/create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,14 @@ export async function createApp({
if (installDeps) {
let packageManager = getPreferredPackageManager();

let npmConfig = execSync(
`${packageManager} config get @remix-run:registry`,
{
let npmConfig;
try {
npmConfig = execSync(`${packageManager} config get @remix-run:registry`, {
encoding: "utf8",
}
);
});
} catch {
// Yarn throws when it can't find a "@remix-run:registry" configuration setting
}
if (npmConfig?.startsWith("https://npm.remix.run")) {
throw Error(
"🚨 Oops! You still have the private Remix registry configured. Please " +
Expand Down
13 changes: 9 additions & 4 deletions packages/remix-dev/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as fse from "fs-extra";
import debounce from "lodash.debounce";
import chokidar from "chokidar";
import { NodeModulesPolyfillPlugin } from "@esbuild-plugins/node-modules-polyfill";
import { pnpPlugin as yarnPnpPlugin } from "@yarnpkg/esbuild-plugin-pnp";
import { NodeResolvePlugin } from "@esbuild-plugins/node-resolve";

import { BuildMode, BuildTarget } from "./build";
import type { RemixConfig } from "./config";
Expand Down Expand Up @@ -316,6 +316,12 @@ async function buildEverything(
}
}

const yarnPnpCompatibilityPlugin = NodeResolvePlugin({
extensions: [".ts", ".js"],
onResolved: (resolved) =>
resolved.includes("node_modules") ? { external: true } : resolved,
});
Comment on lines +319 to +323
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the reasoning behind creating our own plugin instead of using the official one by the Yarn team please?

If changes to the plugin are needed, why don't we create a PR upstream for it?

Copy link
Contributor Author

@lensbart lensbart Jun 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is just an instantiation with default config according to the @esbuild-plugins/node-resolve docs because it’s being used in two places in this file.

I’ve tried using both the previous plugin and this one, and this is the only one that works for me, hence the change.

I haven’t looked into what to fix in the other plugin in order to get it to work.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that this works, but I rather would keep using the official Yarn PnP plugin & create a fix upstream instead of running with our own version (and the burden of maintaining that ourselves) tbh.

Could you please create a compact reproduction repo & file an issue upstream?
That way we can first look on what's the problem on their end & maybe we can fix it in our repo with just a dependency update & without any extra maintenance.

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’ll take a look over the weekend. (Although I still don’t quite understand how this increases our maintenance burden — the plugin is maintained here and is designed specifically for PnP compatibility.)

Copy link
Member

Choose a reason for hiding this comment

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

It's not a big difference, but now we just import the official Yarn PnP plugin and that's it.

With your solution, we have to know the API (& options) of the @esbuild-plugins/node-resolve plugin.
If they ever have a breaking change in there, it adds extra maintenance work on our side.

An API-less plugin won't have any impact then.


async function createBrowserBuild(
config: RemixConfig,
options: BuildOptions & { incremental?: boolean }
Expand Down Expand Up @@ -353,8 +359,7 @@ async function createBrowserBuild(
mdxPlugin(config),
browserRouteModulesPlugin(config, /\?browser$/),
emptyModulesPlugin(config, /\.server(\.[jt]sx?)?$/),
// Must be placed before NodeModulesPolyfillPlugin, so yarn can resolve polyfills correctly
yarnPnpPlugin(),
yarnPnpCompatibilityPlugin, // before `NodeModulesPolyfillPlugin`
NodeModulesPolyfillPlugin(),
];

Expand Down Expand Up @@ -420,7 +425,7 @@ function createServerBuild(
serverEntryModulePlugin(config),
serverAssetsManifestPlugin(assetsManifestPromiseRef),
serverBareModulesPlugin(config, options.onWarning),
yarnPnpPlugin(),
yarnPnpCompatibilityPlugin,
];

if (config.serverPlatform !== "node") {
Expand Down
2 changes: 1 addition & 1 deletion packages/remix-dev/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@
"@babel/preset-env": "^7.18.2",
"@babel/preset-typescript": "^7.17.12",
"@esbuild-plugins/node-modules-polyfill": "^0.1.4",
"@esbuild-plugins/node-resolve": "^0.1.4",
"@npmcli/package-json": "^2.0.0",
"@remix-run/server-runtime": "1.6.2",
"@yarnpkg/esbuild-plugin-pnp": "^2.0.0",
"cacache": "^15.0.5",
"chalk": "^4.1.2",
"chokidar": "^3.5.1",
Expand Down
24 changes: 16 additions & 8 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,16 @@
escape-string-regexp "^4.0.0"
rollup-plugin-node-polyfills "^0.2.1"

"@esbuild-plugins/node-resolve@^0.1.4":
version "0.1.4"
resolved "https://registry.yarnpkg.com/@esbuild-plugins/node-resolve/-/node-resolve-0.1.4.tgz#2257ef3b233c9cb3acd2ebde7d5a3d6874046d38"
integrity sha512-haFQ0qhxEpqtWWY0kx1Y5oE3sMyO1PcoSiWEPrAw6tm/ZOOLXjSs6Q+v1v9eyuVF0nNt50YEvrcrvENmyoMv5g==
dependencies:
"@types/resolve" "^1.17.1"
debug "^4.3.1"
escape-string-regexp "^4.0.0"
resolve "^1.19.0"

"@eslint/eslintrc@^1.0.3":
version "1.0.3"
resolved "https://registry.npmjs.org/@eslint/eslintrc/-/eslintrc-1.0.3.tgz"
Expand Down Expand Up @@ -2334,6 +2344,11 @@
dependencies:
"@types/node" "*"

"@types/resolve@^1.17.1":
version "1.20.2"
resolved "https://registry.yarnpkg.com/@types/resolve/-/resolve-1.20.2.tgz#97d26e00cd4a0423b4af620abecf3e6f442b7975"
integrity sha512-60BCwRFOZCQhDncwQdxxeOEEkbc5dIMccYLwbxsS4TUNeVECQ/pBJ0j09mrHOl/JJvpRPGwO9SvE4nR2Nb/a4Q==

"@types/responselike@*", "@types/responselike@^1.0.0":
version "1.0.0"
resolved "https://registry.npmjs.org/@types/responselike/-/responselike-1.0.0.tgz"
Expand Down Expand Up @@ -2582,13 +2597,6 @@
resolved "https://registry.npmjs.org/@xmldom/xmldom/-/xmldom-0.7.5.tgz"
integrity sha512-V3BIhmY36fXZ1OtVcI9W+FxQqxVLsPKcNjWigIaa81dLC9IolJl5Mt4Cvhmr0flUnjSpTdrbMTSbXqYqV5dT6A==

"@yarnpkg/esbuild-plugin-pnp@^2.0.0":
version "2.0.1"
resolved "https://registry.yarnpkg.com/@yarnpkg/esbuild-plugin-pnp/-/esbuild-plugin-pnp-2.0.1.tgz#120faad903d40e8f000ed3c9db9f9055c9612800"
integrity sha512-M8nYJr8S0riwy4Jgm3ja88m5ZfW6zZSV8fgLtO1mXpwTg0tD9ki1ShPOSm9DEbicc350TVf+k/jVNh6v1xApCw==
dependencies:
tslib "^1.13.0"

"@zxing/[email protected]":
version "0.9.0"
resolved "https://registry.npmjs.org/@zxing/text-encoding/-/text-encoding-0.9.0.tgz"
Expand Down Expand Up @@ -10703,7 +10711,7 @@ tsconfig-paths@^4.0.0:
minimist "^1.2.6"
strip-bom "^3.0.0"

tslib@^1.13.0, tslib@^1.8.1, tslib@^1.9.0:
tslib@^1.8.1, tslib@^1.9.0:
version "1.14.1"
resolved "https://registry.npmjs.org/tslib/-/tslib-1.14.1.tgz"
integrity sha512-Xni35NKzjgMrwevysHTCArtLDpPvye8zV/0E4EyYn43P7/7qvQwPh9BGkHewbMulVntbigmcT7rdX3BNo9wRJg==
Expand Down