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.

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

[Bug?]: Cannot require @yarnpkg/types to use TS in yarn.config.cjs #5878

Closed
1 task
ulrik-s opened this issue Oct 27, 2023 · 6 comments · Fixed by #5954
Closed
1 task

[Bug?]: Cannot require @yarnpkg/types to use TS in yarn.config.cjs #5878

ulrik-s opened this issue Oct 27, 2023 · 6 comments · Fixed by #5954
Labels
bug Something isn't working

Comments

@ulrik-s
Copy link
Contributor

ulrik-s commented Oct 27, 2023

Self-service

  • I'd be willing to implement a fix

Describe the bug

When trying to write typed constraints as shown in the documentation I get an error message. This is behavior is consistent in our mono-repo project as well as newly created empty project.

ulrik@besk:~/github/yarn-test$ yarn add @yarnpkg/types
➤ YN0000: · Yarn 4.0.0
➤ YN0000: ┌ Resolution step
➤ YN0085: │ + @yarnpkg/types@npm:4.0.0, tslib@npm:2.6.2
➤ YN0000: └ Completed in 0s 654ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ 2 packages were added to the project (+ 94.47 KiB).
➤ YN0000: └ Completed in 0s 835ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed
➤ YN0000: · Done in 1s 521ms
ulrik@besk:~/github/yarn-test$ yarn constraints
Internal Error: Cannot find module '@yarnpkg/types'
Require stack:
- /home/ulrik/github/yarn-test/yarn.config.cjs
- /home/ulrik/github/yarn-test/.yarn/releases/yarn-berry.cjs
Require stack:
- /home/ulrik/github/yarn-test/yarn.config.cjs
- /home/ulrik/github/yarn-test/.yarn/releases/yarn-berry.cjs
    at Module._resolveFilename (node:internal/modules/cjs/loader:1048:15)
    at Module._load (node:internal/modules/cjs/loader:901:27)
    at Module.require (node:internal/modules/cjs/loader:1115:19)
    at require (node:internal/modules/helpers:130:18)
    at Object.<anonymous> (/home/ulrik/github/yarn-test/yarn.config.cjs:1:26)
    at Module._compile (node:internal/modules/cjs/loader:1241:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1295:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Module.require (node:internal/modules/cjs/loader:1115:19)
ulrik@besk:~/github/yarn-test$ ls
package.json  yarn.config.cjs  yarn.lock
ulrik@besk:~/github/yarn-test$ cat package.json 
{
  "name": "yarn-test",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "@yarnpkg/types": "^4.0.0"
  }
}
ulrik@besk:~/github/yarn-test$ cat yarn.config.cjs 
const { defineConfig } = require('@yarnpkg/types');

module.exports = defineConfig({
  async constraints({Yarn}) {
    // `Yarn` is now well-typed ✨
  },
});

To reproduce

yarn init -y
yarn set version berry
echo "const { defineConfig } = require('@yarnpkg/types');" > yarn.config.cjs
yarn add @yarnpkg/types
yarn constraints

Environment

System:
    OS: Linux 5.15 Ubuntu 20.04.6 LTS (Focal Fossa)
    CPU: (32) x64 AMD Ryzen 9 3950X 16-Core Processor
  Binaries:
    Node: 20.8.1 - /tmp/xfs-b30aa552/node
    Yarn: 4.0.0 - /tmp/xfs-b30aa552/yarn
    npm: 10.1.0 - /usr/local/bin/npm

Additional context

No response

@ulrik-s ulrik-s added the bug Something isn't working label Oct 27, 2023
@koshic
Copy link

koshic commented Oct 31, 2023

When trying to write typed constraints as shown in the documentation I get an error message

yarn.config.cjs loaded before PnP engine, so currently you can only import types (in PnP mode), not wrapper.

@DavidArchibald
Copy link
Contributor

DavidArchibald commented Nov 8, 2023

I'm not sure the specific reasons that makes defineConfig more useful. Perhaps it's because it'll largely prevent surplus properties? However as an alternative until a fix comes through would be to do:

/** @type {import('@yarnpkg/types').Yarn.Config} */
module.exports = {
  async constraints({Yarn}) {
    // `Yarn` is now well-typed ✨
  },
};

And leave off the require.

Note that @satisfies is now in JSDoc which is an analogy to Typescript's satisfies which should be able to replace @type and also avoid extraneous properties. @satisfies may not be supported for you though.

arcanis added a commit that referenced this issue Nov 13, 2023
…ig (#5954)

**What's the problem this PR addresses?**

We added support for JS constraints thanks to the `yarn.config.cjs`
file. However this file is currently executed from within the Yarn
process, so it doesn't have access to the dependencies when operating
under the PnP installation mode.

Fixes #5878

**How did you fix it?**

I'm still on the fence on the right solution. This PR automatically
loads the `.pnp.cjs` file when the `loadUserConfig` file is called. An
alternative would be a post-install hook to allow each linker to inject
their runtime inside the process post-install, but that was more
involved and I figured it's worth more consideration.

Another alternative was to not do anything and expect users to setup the
PnP runtime themselves, but it feels something that Yarn should be able
to handle.

A third alternative would be to evaluate this file within a worker
thread, which would be started with the proper `execArgv` flags. It's
less intrusive than the post-install hook so I kinda like this idea, but
again - worth more consideration.

I'm working on enabling tests, and to this effect I'm checking whether
we can remove the `NODE_OPTIONS` values from the `makeTemporaryEnv`
subprocesses.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
@milesrichardson
Copy link
Contributor

Here's how I got the expected type support in yarn.config.cjs, with the least headache. There are probably some other ways to do this, but this works for me.

Notes:

  • I'm using the node-modules linker.
  • My top level tsconfig.json has "include": ["yarn.config.cjs"],.
  • Auto-complete in my editor works as expected (the Yarn variable is appropriately auto-completed, and there are no variables typed any), and this passes my strict tsconfig settings

Changes:

  • Annotate the return value of the require function (it would be nice to use import but we can't). Construct its type with two convenience types YarnConfig and DefineConfig, which are not strictly necessary (it could all be done in one giant type annotation) but makes it more readable.
  • Annotate the type of the enforceBlahBlah function to be YarnConfig["constraints"], because otherwise TypeScript can't infer that will be the type (it doesn't know you're going to pass the function to defineConfig).
  • Ensure that function returns Promise<void>, because that's the return type that is specified in YarnConfig["constraints"]. Also, make sure to await when calling it.
  • Delete the // @ts-check annotation since it's unnecessary when I specify the file in the include of my tsconfig.json

First, just for clarity, here's the diff between the yarn.config.cjs in my code and what I got from the documentation:

diff --git a/yarn.config.cjs b/yarn.config.cjs
index 6c3bd36..52823fd 100644
--- a/yarn.config.cjs
+++ b/yarn.config.cjs
@@ -1,12 +1,13 @@
-// @ts-check
-
-/** @type {import('@yarnpkg/types')} */
-const { defineConfig } = require(`@yarnpkg/types`);
+/*** @typedef { import('@yarnpkg/types').Yarn.Config } YarnConfig */
+/*** @typedef { import('@yarnpkg/types').defineConfig } DefineConfig */
+/*** @type { { defineConfig: DefineConfig }} _ */
+const { defineConfig } = require("@yarnpkg/types");
 
 /**
  * This rule will enforce that a workspace MUST depend on the same version of
  * a dependency as the one used by the other workspaces.
  *
+ * @type YarnConfig["constraints"]
  */
 function enforceConsistentDependenciesAcrossTheProject({ Yarn }) {
   for (const dependency of Yarn.dependencies()) {
@@ -24,10 +25,11 @@ function enforceConsistentDependenciesAcrossTheProject({ Yarn }) {
       dependency.update(otherDependency.range);
     }
   }
+  return Promise.resolve();
 }
 
 module.exports = defineConfig({
   constraints: async (ctx) => {
-    enforceConsistentDependenciesAcrossTheProject(ctx);
+    await enforceConsistentDependenciesAcrossTheProject(ctx);
   },
 });

And here's the full code of yarn.config.cjs:

/*** @typedef { import('@yarnpkg/types').Yarn.Config } YarnConfig */
/*** @typedef { import('@yarnpkg/types').defineConfig } DefineConfig */
/*** @type { { defineConfig: DefineConfig }} _ */
const { defineConfig } = require("@yarnpkg/types");

/**
 * This rule will enforce that a workspace MUST depend on the same version of
 * a dependency as the one used by the other workspaces.
 *
 * @type YarnConfig["constraints"]
 */
function enforceConsistentDependenciesAcrossTheProject({ Yarn }) {
  for (const dependency of Yarn.dependencies()) {
    if (dependency.type === `peerDependencies`) {
      continue;
    }

    for (const otherDependency of Yarn.dependencies({
      ident: dependency.ident,
    })) {
      if (otherDependency.type === `peerDependencies`) {
        continue;
      }

      dependency.update(otherDependency.range);
    }
  }
  return Promise.resolve();
}

module.exports = defineConfig({
  constraints: async (ctx) => {
    await enforceConsistentDependenciesAcrossTheProject(ctx);
  },
});

@koshic
Copy link

koshic commented Nov 24, 2023

@milesrichardson it's a bit overcomplicated. defineConfig is designed to provide types without using jsdoc annotations, so simple config (included in tsconfig with "checkJs": true) looks like that:
image

What about your particular example, you just need to use async functions instead of manually created promises:
image

@milesrichardson
Copy link
Contributor

milesrichardson commented Nov 24, 2023

Nice! Thanks :)

One nitpick to arguably make the code a bit safer... Instead of annotating the parameter, I prefer to annotate the whole function (i.e. including the return value), since otherwise I'm losing some information (e.g. what Yarn changes the return type to be something other than undefined? I'd want TypeScript to catch that regression)

Sure, it's true that we're not passing the enforceXxx function directly to the constraints property of the parameter to defineConfig - and if we were doing that, then TypeScript would check the function signature. But we're wrapping the enforcement functions in one function that is basically treating them as the same type as itself. So just to be safe, I prefer to typecheck them as if they were the same type:

/*** @typedef { import('@yarnpkg/types').Yarn.Config["constraints"] } EnforcementFunction */

// ....

/**
 * @type { EnforcementFunction }
 */
function enforceButBeSafeAboutIt({ Yarn }) { /* ... */ }

// ...

module.exports = defineConfig({
  constraints: async (ctx) => {
    await enforceButBeSafeAboutIt(ctx);
  },
});

Here's the type information (although I'm a bit annoyed TypeScript doesn't complain about the lack of returning a Promise):

image

Compare that to the type information we get when only annotating the parameter:

image

@koshic
Copy link

koshic commented Nov 24, 2023

One nitpick to arguably make the code a bit safer...

It's not quite true - code doesn't become more safe: relationship between enforceXxx and constraints: async (ctx) => {. is your internal contract, and you just add useless dependency (full fn signature is wider than parameter type) to it. In case if Yarn types will be changed, you have to modify all the enforceXxx functions instead of one wrapper which be highlighted by TS, which is useless work ) Good pattern is to keep internal contracts independent from external types as much as possible (object shapes instead of function signatures, in that example), just because you can't control external types anyhow. Sure, you can add strict types to the internal contract, but due to above it should be your own types (as an example,EnforcementFunction may return something like { forceStop: boolean } to simulate break logic). Boring mode off )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants