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

[Feature] RFC: TypeScript Constraints #1276

Closed
arcanis opened this issue Apr 30, 2020 · 4 comments · Fixed by #5026
Closed

[Feature] RFC: TypeScript Constraints #1276

arcanis opened this issue Apr 30, 2020 · 4 comments · Fixed by #5026
Labels
enhancement New feature or request rfc This issue is an RFC.

Comments

@arcanis
Copy link
Member

arcanis commented Apr 30, 2020

Describe the user story

I'm a developer working on constraints. There are various problems with the current Prolog syntax:

  • My editor doesn't support it very well. The main VSCode extension doesn't stop crashing unless I install swipl, which I shouldn't have to do since I don't use VSCode otherwise.

  • I don't know which functions are available to me. I can check the online documentation, but I'm a bit lazy and I want to see the documentation directly within my editor. I also want it to let me know if I make a mistake (like passing a string variable to atom/1, or using unknown predicates).

  • I want to compose my predicates, and potentially share them with other developers.

Describe the solution you'd like

Let's first make this clear: we will keep using Prolog. It's proven to be a strong choice so far, especially because of how easily it can be used to query the fact database using yarn constraints query. It's a very powerful tool, and there's no reason to reimplement the wheel.

However, for all its qualities, Prolog is somewhat dated and doesn't benefit from the same tooling support as TypeScript (or even JavaScript). Prettier cannot work on it, typecheckers are mostly non-existing. Even its syntax and APIs are somewhat weird in our world.

Fortunately, I think I have a reasonable solution to this problem: TypeScript constraints. For starter, take a look at the following snippet:

import { t, atom, expect, yarn } from "@yarnpkg/constraints-runtime";

yarn
  .enforceWorkspaceDependency(
    t.WorkspaceCwd,
    t.DependencyName,
    t.DependencyRange,
    t.DependencyType,
  )
  .forEach([
    yarn.workspaceHasDependency(t.WorkspaceCwd, t.DependencyName, _, t.DependencyType),
    yarn.workspaceHasDependency(_, t.DependencyName, t.DependencyRange, t.OtherType),

    expect.notEqual(t.DependencyType, `peerDependencies`),
    expect.notEqual(t.OtherType, `peerDependencies`),
  ]);

yarn
  .enforceWorkspaceDependency(
    t.Workspace,
    t.DependencyIdent,
    t.DependencyRange,
    t.DependencyType,
  )
  .forEach([
    yarn.workspaceHasDependency(t.Workspace, t.DependencyIdent, _, t.DependencyType),
    expect.notEqual(t.DependencyType, `peerDependencies`),

    yarn.workspaceIdent(t.DependencyCwd, t.DependencyIdent),
    yarn.workspaceField(t.DependencyCwd, `version`, t.DependencyVersion),
    expect.exists(t.DependencyVersion),

    expect.notProvable([
      yarn.projectWorkspacesByDescriptor(
        t.DependencyIdent,
        t.DependencyRange,
        t.DependencyCwd,
      ),
    ]),

    when(expect.equal(t.Type, `peerDependencies`), {
      then: [
        atom.concat(`^`, t.DependencyVersion, t.WorkspaceRange),
      ],
      otherwise: [
        atom.concat(`workspace:^`, t.DependencyVersion, t.WorkspaceRange),
      ],
    }),
  ]);

This is Prolog. But it's TypeScript. And because it's TypeScript, it benefits from all the TS tooling, while still having the powerful Prolog semantics that worked so well. Autocomplete will suggest the right functions, typecheck will ensure that we don't pass invalid parameters to our functions (we could then remove those + / - / ? from the documentation), documentation will be displayed on mouse over...

Even better, the syntax would also make it possible to import symbols from third-party packages! Thereby fixing #469, users only having to import predefined predicates from regular dependencies.

Implementation-wise, a few details:

  • The t variable would be a proxy that would accept any name and return it as a variable name. I don't think we can restrict it to upper camelcase symbols, which is the only type problem I can foresee.

  • Not sure whether to use _ or null. Both would work, although null would avoid us having to export a symbol.

  • We would still support the Prolog format, so that even if something isn't possible through the TypeScript syntax, it can still be done using the regular Prolog one.

Describe the drawbacks of your solution

  • I find it quite a bit more verbose than the Prolog version. The IDE support makes it worth it imo.

  • The naysayers will be all "ahah you see that JS was better all along". In fact, they probably stopped reading about two and a half seconds after the title 🙃

Describe alternatives you've considered

We can keep using Prolog. The documentation will have to be improved in either cases, though.

Additional context

cc @bgotink

@arcanis arcanis added the enhancement New feature or request label Apr 30, 2020
@bgotink
Copy link
Sponsor Member

bgotink commented May 8, 2020

I can see why prolog is a hurdle a lot of people are not willing to take, even though imo it is very clearly the right tool for the job. Constraints are declared, hence a declarative language.

Yes, type safety is a nice addition. I would even propose a slightly different syntax that feels more like real javascript/typescript and supports variables. The snippet in the original post doesn't account for variables at all, e.g. typescript doesn't care that t.Type and t.WorkspaceRange in the snippet are mistakes and should be t.DependencyType and t.DependencyRange respectively.

alternative syntax example that's more typesafe
import { _, t, expect, yarn, atom, when } from "@yarnpkg/constraints-runtime";

yarn.enforceWorkspaceDependency((
  workspaceCwd,
  dependencyName,
  dependencyRange,
  dependencyType,
) => {
  const {otherType} = t;

  yarn.workspaceHasDependency(workspaceCwd, dependencyName, _, dependencyType);
  yarn.workspaceHasDependency(_, dependencyName, dependencyRange, otherType);

  expect.notEqual(dependencyType, `peerDependencies`);
  expect.notEqual(otherType, `peerDependencies`);
});

yarn.enforceWorkspaceDependency((
  workspace,
  dependencyIdent,
  dependencyRange,
  dependencyType,
) => {
  yarn.workspaceHasDependency(workspace, dependencyIdent, _, dependencyType);
  expect.notEqual(dependencyType, `peerDependencies`);

  const {dependencyCwd, dependencyVersion} = t;

  yarn.workspaceIdent(dependencyCwd, dependencyIdent);
  yarn.workspaceField(dependencyCwd, `version`, dependencyVersion);
  expect.exists(dependencyVersion);

  expect.notProvable(() => {
    yarn.projectWorkspacesByDescriptor(
      dependencyIdent,
      dependencyRange,
      dependencyCwd,
    );
  });

  when(expect.equal(dependencyType, `peerDependencies`), {
    then: () => {
      atom.concat(`^`, dependencyVersion, dependencyRange);
    },
    otherwise: () => {
      atom.concat(`workspace:^`, dependencyVersion, dependencyRange);
    },
  });
});

Now on to some remarks:

  • Allow me to get this one out there: seeing these snippets, including the one I wrote, makes the computer scientist in me question his sanity. Or it may be the burnout talking, who knows anymore.

  • The current proposal doesn't solve the +/-/? problem at all. Variables can still be instantiated or not, and the functions provided by the yarn package will still need to declare that somehow.
    The only typescript solution for this is different types for instantiated variables. This works, until you have a predicate that instantiates multiple variables. Examples of those: atom.concat, yarn.workspaceHasDependency etc.

    quick POC for variable instantiation

    This works:

    declare type Var = {__var: true}
    declare type InstantiatedVar = Var & {__inst: true}
    
    declare let v: Var;
    declare let i: InstantiatedVar;
    
    declare function atomConcat(prefix: Var, suffix: InstantiatedVar, whole: InstantiatedVar):
        asserts prefix is InstantiatedVar;
    declare function atomConcat(prefix: InstantiatedVar, suffix: Var, whole: InstantiatedVar):
        asserts suffix is InstantiatedVar;
    declare function atomConcat(prefix: InstantiatedVar, suffix: InstantiatedVar, whole: Var): 
       asserts whole is InstantiatedVar;
    
    function test1() {
      atomConcat(v, i, i);
      i = v; // typescript doesn't complain because v is now InstantiatedVar
    }
    
    function test2() {
      atomConcat(i, v, i);
      i = v; // typescript doesn't complain because v is now InstantiatedVar
    }
    
    function test3() {
      atomConcat(i, i, v);
      i = v; // typescript doesn't complain because v is now InstantiatedVar
    }

    but this is the actual signature of the example's atomConcat function:

    declare function atomConcat(prefix: Var, suffix: Var, whole: InstantiatedVar):
        asserts prefix is InstantiatedVar && suffix is InstantiatedVar;
    declare function atomConcat(prefix: InstantiatedVar, suffix: InstantiatedVar, whole: Var):
        asserts whole is InstantiatedVar;
    
    // invalid typescript :(

    however, we could workaround this using the expect.exists function, which would have a signature

    declare function expectExists(v: Var) asserts v is InstantiatedVar;
    
    declare let v: Var, w: Var, i: InstantiatedVar;
    function test4() {
      atomConcat(v, w, i);
      i = v; // typescript doesn't complain because v is now InstantiatedVar
    
      i = w; // no can do
    
      expectExists(w);
    
      i = w; // can do
    }

    Is this ideal? No, not by a long shot. Does it work? Yeah.

  • If I can write constraints in typescript, can I import a helper function from package X? No, you cannot. The following is impossible:

    import semver from 'semver';
    // other imports
    
    // inside a rule
    when(semver.valid(t.DependencyRange), {
      then: [
        // rule for when the dependency range is a single version
      ],
      otherwise: [
        // rule for when the dependency range isn't a single version
      ],
    });

    Constraints in typescript will make it possible to share and reuse constraints, but it will not make it possible to use non-constraint functions inside the constraints file without us providing an escape hatch into imperative code. That imperative code will also be typescript/javascript, so I hope that won't compound on the already existing cognitive load of writing declarative code in an imperative language.

  • Yarn would end up supporting two systems for constraints. While the current proposal could be implemented by generating prolog code from the typescript constraints, I think that's not an ideal solution because a) that still exposes things like prolog's questionable error messages (throw(error(instantiation_error, '/'(atom_concat, 3))) what did I do wrong?) and b) that makes imperative escape hatches harder to implement.

@bgotink
Copy link
Sponsor Member

bgotink commented Nov 21, 2020

I've been thinking about this lately (a lot) and I'm still more in favour of using imperative constraints than declarative if we're going the typescript/javascript route.
Looking back at the proposals above, my feeling is that for non-trivial constraints the declarative syntax will be too complex to grok for anyone who doesn't realise this is prolog posing as typescript—and it's going to be equally hard to get right for the people who do realise.

Below you'll find yarn's constraints written using an imperative API. Not only is it shorter in LOC, but it's "just plain javascript" which everyone understands[citation needed]. There are no caveats about the control flow, no "you can't use if-checks".

import {getWorkspaces, getWorkspaceByIdent, DependencyType} from '@yarnpkg/constraints-runtime';

/**
 * @param {import('@yarnpkg/constraints-runtime').Workspace} workspace 
 */
function *getAllRealDependencies(workspace) {
  yield *workspace.getDependencies(DependencyType.Regular);
  yield *workspace.getDependencies(DependencyType.Dev);
}

const workspaces = await getWorkspaces();
const cliWorkspace = await getWorkspaceByIdent('@yarnpkg/cli');
/** @type {string[]} */
const includedPlugins = cliWorkspace.getManifest().getProperty('@yarnpkg/builder', 'bundles', 'standard').value;

const identsWithOtherBuildSystems = [
  // Pure JS, no build system
  '@yarnpkg/eslint-config',
  // Compiled inline
  '@yarnpkg/libui',
  // Custom webpack build
  '@yarnpkg/pnp',
];

for (const workspace of workspaces) {
  for (const otherWorkspace of workspaces) {
    if (workspace === otherWorkspace)
      continue;

    const dependencies = workspace.getDependencies(DependencyType.Regular);
    const peerDependencies = workspace.getDependencies(DependencyType.Peer);
    const devDependencies = workspace.getDependencies(DependencyType.Dev);
    
    // This rule will enforce that a workspace MUST depend on the same version of a dependency as the one used by the other workspaces
    for (const [dependencyIdent, dependencyRange] of getAllRealDependencies(otherWorkspace)) {
      dependencies.requireRangeIfPresent(dependencyIdent, dependencyRange);
      devDependencies.requireRangeIfPresent(dependencyIdent, dependencyRange);
    }

    // This rule will prevent workspaces from depending on non-workspace versions of available workspaces
    dependencies.requireRangeIfPresent(otherWorkspace.ident, `workspace:^${otherWorkspace.version ?? '*'}`);
    peerDependencies.requireRangeIfPresent(otherWorkspace.ident, `^${otherWorkspace.version ?? '*'}`);
    devDependencies.requireRangeIfPresent(otherWorkspace.ident, `workspace:^${otherWorkspace.version ?? '*'}`);
  }

  // This rule enforces that all packages must not depend on inquirer - we use enquirer instead
  for (const deps of [dependencies, peerDependencies, devDependencies])
    deps.forbidDependency('inquirer');

  // This rule enforces that all packages that depend on TypeScript must also depend on tslib
  if (dependencies.hasDependency('typescript') || devDependencies.hasDependency('typescript'))
    dependencies.requireDependency('tslib'); // no range, just to say it's required

  const manifest = workspace.getManifest();

  // This rule will enforce that all packages must have a "BSD-2-Clause" license field
  manifest.getProperty('license').requireValue('BSD-2-Clause');

  // This rule will enforce that all packages must have a engines.node field of >=10.19.0
  manifest.getProperty('engines', 'node').requireValue('>=10.19.0');

  // Required to make the package work with the GitHub Package Registry
  manifest.getProperty('repository').requireValue({
    type: 'git',
    url: 'ssh://git@github.com/yarnpkg/berry.git',
  });

  // This rule will require that the plugins that aren't embed in the CLI list a specific script that'll
  // be called as part of our release process (to rebuild them in the context of our repository)
  if (workspace.ident.startsWith('@yarnpkg/plugin-') && !includedPlugins.includes(workspace.ident))
    // Simply require that the script has to be present
    manifest.getProperty('scripts', 'update-local').requireType('string');
  
  if (!manifest.getProperty('private').value && !identsWithOtherBuildSystems.includes(workspace.ident)) {
    manifest.getProperty('scripts', 'prepack').requireValue('run build:compile "$(pwd)"');
    manifest.getProperty('scripts', 'postpack').requireValue('rm -rf lib');
  }
}

With regards to implementation:

  • I suggest to only support javascript files to start, supporting typescript is an extra hurdle that makes things more complicated than they need to be for a first iteration. Nothing's stopping users from using // @ts-check and our package would of course be published with typings
    • This does mean the code sample above can only work in non-PnP environments atm, because PnP doesn't support ESM yet. (I chose an ESM example to not have the overhead of an async IIFE.)
  • To allow PnP to work its magic and support require calls in the constraints file, we'll probably have to run the file in a separate process. Using childProcess.fork we can communicate with this fork and send the workspace info and any failures from yarn to the constraints process and back.
  • I've got some fun ideas to ship the runtime to ensure the installed version is compatible with the yarn binary. This would make the typescript API of the @yarnpkg/constraints-runtime package the only important part to consider for breaking changes, while a regular package that needs to be installed will also make the protocol for communication between yarn and the constraints file part of the API surface.

@wdfinch
Copy link

wdfinch commented Oct 23, 2021

I am totally in favor of this. I think what would be really useful is to just export a bunch of common constraints as functions that we could then call and pass in some arguments for customization.

I also like the imperative approach more. I get that the declarative syntax is elegant and concise, however I think it's must harder to debug and figure out what's going on. Given constraints are something I would probably write at the the start of a project and not revisit that often, I would rather have easy to understand although verbose code and I can modify easily rather than spend a few hours trying to remember prolog syntax.

@paul-soporan paul-soporan added the rfc This issue is an RFC. label Sep 16, 2022
@arcanis
Copy link
Member Author

arcanis commented Nov 3, 2022

I started an implementation here: #5026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rfc This issue is an RFC.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants