chore: Migrate constraints from Prolog to JavaScript #47

sirtimid · 2024-09-05T20:20:53Z

Explanation

Migrated constrains. This PR was used as a base with various customizations to align with our constraints.

See comments for adjustments to the prolog constraints.

References

Fixes #17

sirtimid · 2024-09-05T20:22:23Z

packages/shims/package.json

  "module": "./dist/index.mjs",
  "files": [
-    "dist"
+    "dist/"


changed this so we can apply the constrain to this package as well even though the package is private

sirtimid · 2024-09-05T20:22:28Z

packages/streams/package.json

  "types": "./dist/index.d.cts",
  "files": [
-    "dist"
+    "dist/"


changed this so we can apply the constrain to this package as well even though the package is private

sirtimid · 2024-09-05T20:23:21Z

yarn.config.cjs

Click Load diff to see the file since it is big

sirtimid · 2024-09-05T20:24:33Z

yarn.config.cjs

+// Packages that do not have an entrypoint, types, or sideEffects
+const entrypointExceptions = ['shims', 'streams'];
+// Packages that do not have typedoc
+const typedocExceptions = ['test-utils', 'extension'];
+// Packages that do not have build or tests
+const noBuildOrTests = ['test-utils'];
+// Packages that do not export a `package.json` file
+const noPackageJson = ['extension'];


We set the exceptions at the top so we can edit them easily

sirtimid · 2024-09-05T20:36:14Z

yarn.config.cjs

+      expectValidVersionRanges(Yarn, workspace);
+
+      // Ensure dependency ranges are synchronized across the monorepo
+      expectSynchronizedRanges(Yarn, workspace);
+
+      // Ensure dependencies are not duplicated across dependency types
+      expectUniqueDependencyTypes(Yarn, workspace);


these weren't existent in the core example so if you want to check the code I wrote.

sirtimid · 2024-09-05T20:36:50Z

yarn.config.cjs

+      // If one workspace package lists another workspace package within
+      // `peerDependencies`, the dependency range must satisfy the current
+      // version of that package.
+      expectUpToDateWorkspacePeerDependencies(Yarn, workspace);


this was in the core example but not in our constraints, I figured it is ok we leave it

sirtimid · 2024-09-05T20:37:16Z

yarn.config.cjs

+      // `dependencies`, and B is a controller package, then we need to ensure
+      // that B is also listed in A's `peerDependencies` and that the version
+      // range satisfies the current version of B.
+      expectControllerDependenciesListedAsPeerDependencies(


this also was in the MetaMask/core#4546 but not in our constraints

"Controllers" are specific to the core monorepo, so we should delete both this call and the function itself.

sirtimid · 2024-09-05T20:38:03Z

yarn.config.cjs

+      if (!isPrivate) {
+        // All non-root packages must have a valid README.md file.
+        await expectReadme(workspace, workspaceBasename);
+      }


this also was in the MetaMask/core#4546 but not in our constraints, we don't have any readme but it is ok to leave it here for when they are not private anymore

Yeah I think this is okay. The body of the function is also okay, because eventually we will prefix all of these packages with @metamask/. For now we prefix them with @ocap/ just as a convenience. (That org name is also taken 😢 )

rekmarks

Looks good! I left a couple of inline suggestions. In addition to those, I noticed that the /core version of the config file has type checking enabled. Our TypeScript configuration is quite different from theirs, so I decided to gauge how difficult it might be to enable. As it turns out, it shouldn't be bad at all! Therefore, I think we should do this too.

#48 contains the result of my experiment. The only problem remaining is a set of errors related to types that I believe are defined directly in the file, and therefore shouldn't be difficult to fix. Feel free to merge my branch or reimplement my changes.

rekmarks · 2024-09-05T23:53:01Z

yarn.config.cjs

+      // `dependencies`, and B is a controller package, then we need to ensure
+      // that B is also listed in A's `peerDependencies` and that the version
+      // range satisfies the current version of B.
+      expectControllerDependenciesListedAsPeerDependencies(


"Controllers" are specific to the core monorepo, so we should delete both this call and the function itself.

rekmarks · 2024-09-05T23:57:20Z

yarn.config.cjs

+      if (!isPrivate) {
+        // All non-root packages must have a valid README.md file.
+        await expectReadme(workspace, workspaceBasename);
+      }


Yeah I think this is okay. The body of the function is also okay, because eventually we will prefix all of these packages with @metamask/. For now we prefix them with @ocap/ just as a convenience. (That org name is also taken 😢 )

This is a half-finished attempt to enable `// @ts-check` in `yarn.config.js`. See review for more details.

sirtimid · 2024-09-06T10:01:48Z

Looks good! I left a couple of inline suggestions. In addition to those, I noticed that the /core version of the config file has type checking enabled. Our TypeScript configuration is quite different from theirs, so I decided to gauge how difficult it might be to enable. As it turns out, it shouldn't be bad at all! Therefore, I think we should do this too.

#48 contains the result of my experiment. The only problem remaining is a set of errors related to types that I believe are defined directly in the file, and therefore shouldn't be difficult to fix. Feel free to merge my branch or reimplement my changes.

@rekmarks ok, all fixed. Remember to yarn install so you won't have the lodash error.

rekmarks

@sirtimid the // @ts-expect-error I had for the lodash import was not due to a missed yarn install, but because lodash does not export a valid ESM module (error ts(2306)). Were you getting an error on the "expect error" directive on your end? If so, we could replace it with a // @ts-ignore since this is neither a source nor TS file, and we don't have to be that careful with the types.

Either way, LGTM, and I'll leave it to you whether to address the TS error.

sirtimid · 2024-09-06T18:08:12Z

@sirtimid the // @ts-expect-error I had for lodash was not due to a missed yarn install, but because lodash does not export a valid ESM module (error ts(2306)). Were you getting an error on the "expect error" directive on your end? If so, we could replace it with a // @ts-ignore since this is neither a source nor TS file, and we don't have to be that careful with the types.

Either way, LGTM, and I'll leave it to you whether to address the TS error.

@rekmarks Ah, I don't get the error because I have the @types/lodash locally and I removed it from deps since lint was complaining that it is unused. I don't want to change the tsconfig to allowJS so I ll just add the ignore

socket-security · 2024-09-06T18:14:30Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@types/lodash@4.17.7	None	`0`	867 kB	types

View full report↗︎

Dimitrios Marlagkoutsos and others added 2 commits September 5, 2024 21:09

Migrate prolog constraints to yarn with javascript

151ea94

Document exceptions

818be8e

sirtimid requested a review from a team as a code owner September 5, 2024 20:20

sirtimid commented Sep 5, 2024

View reviewed changes

yarn.config.cjs

Copy link

Contributor Author

sirtimid Sep 5, 2024

Choose a reason for hiding this comment

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

Click Load diff to see the file since it is big

sirtimid commented Sep 5, 2024

View reviewed changes

fix comment

058b9c9

sirtimid commented Sep 5, 2024

View reviewed changes

remove unused

f47a71a

rekmarks self-requested a review September 5, 2024 20:45

rekmarks requested changes Sep 6, 2024

View reviewed changes

rekmarks and others added 3 commits September 6, 2024 10:52

refactor: (WIP) Enable ts-check in yarn.config.js (#48)

7229d21

This is a half-finished attempt to enable `// @ts-check` in `yarn.config.js`. See review for more details.

fix types

969baec

remove redundant

ea980a8

sirtimid requested a review from rekmarks September 6, 2024 10:01

rekmarks previously approved these changes Sep 6, 2024

View reviewed changes

Add lodash types

c409381

sirtimid dismissed rekmarks’s stale review via c409381 September 6, 2024 18:13

sirtimid enabled auto-merge (squash) September 6, 2024 18:14

rekmarks approved these changes Sep 6, 2024

View reviewed changes

sirtimid merged commit 4e4d6f8 into main Sep 6, 2024

sirtimid deleted the sirtimid/prolog-to-javascript-constraints branch September 6, 2024 18:18

chore: Migrate constraints from Prolog to JavaScript #47

chore: Migrate constraints from Prolog to JavaScript #47

Uh oh!

Conversation

sirtimid commented Sep 5, 2024

Explanation

References

Uh oh!

sirtimid Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirtimid Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rekmarks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirtimid commented Sep 6, 2024

Uh oh!

rekmarks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirtimid commented Sep 6, 2024

Uh oh!

socket-security bot commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sirtimid Sep 5, 2024 •

edited

Loading

sirtimid Sep 5, 2024 •

edited

Loading

rekmarks left a comment •

edited

Loading

rekmarks left a comment •

edited

Loading