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

Define spec for development environment field #15

Open
GeoffreyBooth opened this issue Feb 27, 2024 · 95 comments
Open

Define spec for development environment field #15

GeoffreyBooth opened this issue Feb 27, 2024 · 95 comments

Comments

@GeoffreyBooth
Copy link

GeoffreyBooth commented Feb 27, 2024

This is a draft spec for nodejs/node#51888 (comment), to define the runtime and package manager for developing a project or package (not consuming/installing it as a dependency in another project). We could use the name devEngines and it would be a new top-level field defined with this schema:

interface DevEngines {
  os?: DevEngineDependency | DevEngineDependency[];
  cpu?: DevEngineDependency | DevEngineDependency[];
  runtime?: DevEngineDependency | DevEngineDependency[];
  packageManager?: DevEngineDependency | DevEngineDependency[];
}

interface DevEngineDependency {
  name: string;
  version?: string;
  onFail?: 'ignore' | 'warn' | 'error' | 'download';
  download?: {
    url: string;
    algorithm?: string;
    digest?: string;
  }
}

The os, cpu, runtime, and packageManager sub-fields could either be an object or an array of objects, if the user wants to define multiple acceptable OSes, CPUs, runtimes or package managers. The first acceptable option would be used, and onFail would be triggered for the last defined option if none validate. If unspecified, onFail defaults to error for the non-array notation; or it defaults to error for the last element in the array and ignore for all prior elements, for the array notation. Validation would check the name and version ranges.

The name field would be a string, corresponding to different sources depending on the parent field:

The version field syntax would match that defined for engines.node, so something like ">= 16.0.0 < 22" or ">= 20". If unspecified, any version matches.

The onFail field defines what should happen if validation fails:

  • ignore: nothing.
  • warn: print something and continue.
  • error: print something and exit.
  • download: remediate the validation failure by downloading the requested tool/version.

The download field would apply only for onFail: 'download'. I assume no one would support download for os, cpu or runtime. It could also be supported on a case-by-case basis, like perhaps Yarn and pnpm could support downloading a satisfactory version while npm would error. The optional algorithm and digest fields would allow for verifying a download, per the https://w3c.github.io/webappsec-subresource-integrity spec.

Typical example:

"devEngines": {
  "runtime": {
    "name": "node",
    "version": ">= 20.0.0",
    "onFail": "error"
  },
  "packageManager": {
    "name": "yarn",
    "version": "3.2.3",
    "onFail": "download",
    "download": {
      "url": "https://registry.npmjs.org/@yarnpkg/cli-dist/-/cli-dist-3.2.3.tgz",
      "algorithm": "sha224",
      "digest": "16a0797d1710d1fb7ec40ab5c3801b68370a612a9b66ba117ad9924b"
    }
  }
}

“Uses every possible field” example:

"devEngines": {
  "os": {
    "name": "darwin",
    "version": ">= 23.0.0"
  },
  "cpu": [
    {
      "name": "arm"
    }, {
      "name": "x86"
    }
  ],
  "runtime": [
    {
      "name": "bun",
      "version": ">= 1.0.0",
      "onFail": "ignore"
    },
    {
      "name": "node",
      "version": ">= 20.0.0",
      "onFail": "error"
    },
  ],
  "packageManager": [
    {
      "name": "bun",
      "version": ">= 1.0.0",
      "onFail": "ignore"
    },
    {
      "name": "yarn",
      "version": "3.2.3",
      "onFail": "download",
      "download": {
        "url": "https://registry.npmjs.org/@yarnpkg/cli-dist/-/cli-dist-3.2.3.tgz",
        "algorithm": "sha224",
        "digest": "16a0797d1710d1fb7ec40ab5c3801b68370a612a9b66ba117ad9924b"
      }
    }
  ]
}

Some potential future expansions of this spec that have been discussed are:

  • runtime and packageManager might take shorthand string values defining the desired name or name with version/version range.
@ljharb

This comment was marked as resolved.

@wesleytodd

This comment was marked as resolved.

@GeoffreyBooth

This comment was marked as outdated.

@wraithgar

This comment has been minimized.

@GeoffreyBooth
Copy link
Author

onFail is a user directive, not a maintainer directive.

But in this case, the user is the maintainer. This whole configuration block only applies when it’s the developer of the package/project.

@wraithgar
Copy link

How does download even work with a semver range? If these are all npm registry packages then there is already a well defined discovery mechanism for downloading a given named package at a given version.

I'd love to hear from yarn or pnpm if and how they'd plan on handling the failure state where the user has indicated they want to download the correct version. How do you pick the version? Do you spawn it yourself? Why not fail and let the user remediate?

@wraithgar
Copy link

But in this case, the user is the maintainer

Good point. As you can see the lines between "installing as module" and "interacting as app" are so blurry that even I missed that.

@GeoffreyBooth
Copy link
Author

GeoffreyBooth commented Feb 27, 2024

How does download even work with a semver range?

If a URL is provided, we’re just downloading that exact URL. The version constraint wouldn’t apply.

If we want to allow onFail: 'download' and a missing download field, the only behavior I can think of would be to query the npm registry for the requested package manager and download the newest version that fits within the range.

@wesleytodd

This comment was marked as resolved.

@GeoffreyBooth

This comment was marked as resolved.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2024

This is a great start. I think it's important that it not just be limited to the runtime and the package manager - process.versions has a lot of things in it that would be really nice to also constrain in various cases, especially openssl which has lots of CVEs.

@styfle
Copy link
Contributor

styfle commented Feb 27, 2024

Tagging relevant people to review the draft spec: @Ethan-Arrowood @arcanis @zkochan @aduh95

@wraithgar
Copy link

I am not very good at reading specs like this. What would it look like in implementation to limit the local dev environment to a given os or cpu for example? From what I see it doesn't look like that field is sufficiently decoupled from "named" which doesn't make sense in that context.

@aduh95
Copy link

aduh95 commented Feb 27, 2024

packageManager is already used in the wild as a string, we'd need to define what happens when the value is a string to allow a smooth transition – another option is to use a different key, but packageManager is quite good a describing what it is.

@wesleytodd
Copy link
Collaborator

wesleytodd commented Feb 27, 2024

but packageManager is quite good a describing what it is .... is already used in the wild as a string

This proposal covers way more than package manager and would not be under that field name (although it is not mentioned above). I think that should have always been the goal, so that is why keeping the field "experimental" in node core was important. I agree we would want to smooth the path. But I think that is up to the folks who implemented the field in their tools. Maybe here we would just discuss goals for ways to make that transition?

@ljharb
Copy link
Member

ljharb commented Feb 27, 2024

The first step should be figuring out (such that they can be clearly documented and conveyed) the problems we're solving, and the goals and non goals - and then to sketch out a solution that seems viable to all stakeholders.

At that point, those interested in the migration path from the experimental packageManager field to the new thing can explore the best way to achieve that - but until the solution design is settled I'm not sure how any progress could be made on migration.

@Ethan-Arrowood
Copy link
Collaborator

Cross-posting from Slack:

I’ve attempted to read all of the various GitHub and Twitter threads that have led us here. I think there is a lot going on in general, and I don’t have a clear, concise reply just yet. For now, all I will say is that I encourage all invested parties to join the monthly call on March 5th. We rarely have much to discuss regarding the group’s active work streams, so we can reasonably spend the entire hour discussing the corepack issue.

For those looking for the upcoming meeting invite, please copy the invite from the OpenJS Calendar

@GeoffreyBooth
Copy link
Author

What would it look like in implementation to limit the local dev environment to a given os or cpu for example?

I added those fields because you had mentioned them on the other thread. I'm not sure what the use case would be; run npm on Windows and Bun on macOS? I can just remove them if you can't think of a use case either. We can always add fields later.

@ljharb
Copy link
Member

ljharb commented Feb 28, 2024

I think that os/cpu etc are in the category of "general engines worth explicitly giving a range for", and altho "runtime" and "package manager" are reasonable to call special, the schema needs a mostly free-for-all dumping ground too (like engines, import.meta, etc are all now) for things like this.

@wraithgar
Copy link

Yeah, remember we're defining a spec for all development runtime constraints, not just package manager. Package manager is a subset of constraints. Node version, os, cpu, and libc are other constraints.

@zkochan
Copy link

zkochan commented Feb 28, 2024

Why would you allow specifying multiple different package managers? I assume there are still people in this group that are strongly against lockfiles. But at least don't motivate devs to use different package managers on a project. It will make everyone's life a lot harder.

@wesleytodd
Copy link
Collaborator

wesleytodd commented Feb 28, 2024

Why would you allow specifying multiple different package managers?

My guess is this is done to remain flexible, not make a recommendation that folks should. That said, I could think of a few cases where maybe you want to use pnpm for local dev but npm for publish or something of that nature?

I assume there are still people in this group that are strongly against lockfiles.

I don't think that would be a driving part of the decision is it? I assume you are meaning that if a package declared multiple dev package managers they would then have to resolve the lockfile differences? I think at this layer of abstraction (a spec for the package.json) we would not want to exclude package managers from agreeing on a shared lockfile format (in fact we might want to encourage that 😎) so I think it is reasonable to keep.

@styfle
Copy link
Contributor

styfle commented Feb 28, 2024

How about changing “hash” into two fields: “algorithm” and “digest”?

@GeoffreyBooth
Copy link
Author

How about changing “hash” into two fields: “algorithm” and “digest”?

That’s fine, what would be an example? This?

"download": {
  "url": "https://registry.npmjs.org/@yarnpkg/cli-dist/-/cli-dist-3.2.3.tgz",
  "algorithm": "sha224",
  "digest": "16a0797d1710d1fb7ec40ab5c3801b68370a612a9b66ba117ad9924b"
}

@wesleytodd
Copy link
Collaborator

wesleytodd commented Feb 28, 2024

algorithm and digest should also be optional (but encouraged for the implementing tools to add/update them).

@wraithgar
Copy link

What's the "hash" currently? In npm lockfiles it's the output of ssri.stringify which includes both algorithm and digest

@wesleytodd
Copy link
Collaborator

wesleytodd commented Feb 28, 2024

For folks who need it (me) here is what ssri references: https://www.npmjs.com/package/ssri

EDIT: I should have added, this is an implementation of the subresource integrity spec used in browsers as well: https://w3c.github.io/webappsec-subresource-integrity/

So with that in mind, I would say that seems like it could be a good spec to just directly here.

@Ethan-Arrowood
Copy link
Collaborator

I see no harm in having additional, optional properties.

We should specify other things like if non-specified properties are allowed or not. This will be important when we get a JSON schema established.

@GeoffreyBooth
Copy link
Author

GeoffreyBooth commented Mar 14, 2024

os, cpu, and libc being top level fields were a result of this being the “local dev” equivalent of the existing attributes, along with engines in package.json. Omitting them fails that goal. I’m not sure what the pushback is on putting these fields (which already have well defined values and are in use already for installing packages) into this new context.

I understand what those fields mean in the engines context, where something like "os": ["win32"] means that this package will only work when installed by Windows consumers. But what would a devEngines.os field mean? That development is only allowed in the listed OSes? What’s the use case for that?

The original proposal suggested os and cpu as fields under runtime and packageManager, to allow for defining configuration like “Run in Node in Windows, Bun elsewhere; and always use pnpm as the package manager.” That’s at least a use case that I can imagine someone having, but I don’t think it’s common enough that it needs to be part of MVP.

We should specify other things like if non-specified properties are allowed or not.

If we disallow additional properties, wouldn’t that mean that we can’t extend this spec in the future?

@ljharb
Copy link
Member

ljharb commented Mar 14, 2024

That development is only allowed in the listed OSes? What’s the use case for that?

yes, plenty of repos simply can't be developed on windows, or only can be developed on windows, for various reasons.

@wraithgar
Copy link

But what would a devEngines.os field mean? That development is only allowed in the listed OSes? What’s the use case for that?

Just that. Development only works in that given environment. Feel free to leave that out of the spec, npm will probably be including it in our own implementation as a non-standard extension though.

Again, given the problem we're solving is "engines etc, but for local dev" leaving those out doesn't seem appropriate.

I think this is close enough that we can start implementation, hopefully a PR can shape up by next week, probably in npm-install-checks

@wesleytodd
Copy link
Collaborator

Just that. Development only works in that given environment. Feel free to leave that out of the spec, npm will probably be including it in our own implementation as a non-standard extension though.

The goal of this is to avoid exactly this comment. I don't think it is valuable to leave it out if you are going to implement it and there is clearly a use case.

I think this is close enough that we can start implementation, hopefully a PR can shape up by next week

Did you see the conversation we were having in slack? I don't believe we have addressed everything.

@wraithgar
Copy link

No sorry @wesleytodd I didn't see it. I still think a draft PR will help since a lot of the effort on our side is going to be implementation specific. The object itself can change format easily. It will be a draft PR with the purpose of helping npm shake out exactly where this code needs to live and what its api is in npm itself.

@wraithgar
Copy link

Conversations in other channels are fine, to a point. Not everyone is in every slack though so if folks can at least summarize important discussions here that'd be swell.

@wesleytodd
Copy link
Collaborator

I still think a draft PR will help since a lot of the effort on our side is going to be implementation specific.

I am hoping to avoid extra work for y'all honestly.

Conversations in other channels are fine, to a point. Not everyone is in every slack though so if folks can at least summarize important discussions here that'd be swell.

Yes, strong agree. The problem has been these threads have been too long to keep up with and add to that everyone frustration with this continuing discussion I didn't want to brainstorm in the thread. We can brain storm in another thread or issue on GH which is fine. But this thread is not the one either for that. Honestly on a VC is the right place because my things were questions and thoughts to work through. If you had npm rfc calls I would discuss there 😉. Without those, tell me where you want me to go?

@wraithgar
Copy link

Thanks for the feedback @wesleytodd. We'll hold off on implementation then.

@GeoffreyBooth
Copy link
Author

Just that. Development only works in that given environment.

What’s the use case for this, though? It makes sense for engines, to tell the consuming user that the library only supports a particular OS, so if they’re on an unsupported OS then they should find a different library; but why would we want to tell a library’s maintainer what its supported OSes are? What’s an example where someone would define this field?

@ljharb
Copy link
Member

ljharb commented Mar 19, 2024

@GeoffreyBooth many of my packages' dev-time run-scripts don't work on windows. They use env vars and not cross-env, or they use || or other shell tricks, etc.

@wesleytodd
Copy link
Collaborator

@GeoffreyBooth many of my packages' dev-time run-scripts don't work on windows. They use env vars and not cross-env, or they use || or other shell tricks, etc.

This is VERY common and today we don't have a way for folks to indicate it and fail, users of windows just have to know they may not be able to work on projects easily. I do think this is an important use case to accomidate for.

@GeoffreyBooth
Copy link
Author

This is VERY common and today we don’t have a way for folks to indicate it and fail, users of windows just have to know they may not be able to work on projects easily.

Okay, that makes sense, I can add it. What about also having it as a field within runtimes and packageManager? You might very well want to define differing runtimes per OS, like you could say “overall development is allowed in Mac and Windows only; for runtimes, use Bun in Mac and Node in Windows”.

@wraithgar
Copy link

wraithgar commented Mar 19, 2024

What about also having it as a field within runtimes and packageManager

I've never seen that specific limitation, only a broader one for OS, etc in general.

ETA: The reasons for folks needing os/libc/cpu are usually because the packages being developed are FOR those endpoints. The node version and package manager is irrelevant there.

@MrHBS
Copy link

MrHBS commented May 19, 2024

Any updates here boys?

@wraithgar
Copy link

Sorry for the radio silence. There was a bit of a miscommunication about whether this was ready for npm to implement, coupled w/ some other things in April that tore our focus away.

Unless I hear otherwise this is the spec npm is going to work towards implementing, download notwithstanding:

interface DevEngines {
  os?: DevEngineDependency | DevEngineDependency[];
  cpu?: DevEngineDependency | DevEngineDependency[];
  runtime?: DevEngineDependency | DevEngineDependency[];
  packageManager?: DevEngineDependency | DevEngineDependency[];
}
interface DevEngineDependency {
  name: string;
  version?: string;
  onFail?: 'ignore' | 'warn' | 'error' | 'download';
  download?: {
    url: string;
    algorithm?: string;
    digest?: string;
  }
}

The package.json attribute npm will look for is going to be devEngines

cc @zkochan

@ljharb
Copy link
Member

ljharb commented May 30, 2024

what's the default of onFail? also, why is version optional?

@wraithgar
Copy link

Version is optional to allow folks to define the package manager in general, not a specific version. Functionally it's the same as defaulting it to "*".

@wesleytodd
Copy link
Collaborator

I thought we had agreed not to do the parts other than name and version? The whole onFail and download stuff is IMO preemptive still.

@GeoffreyBooth
Copy link
Author

I thought we had agreed not to do the parts other than name and version? The whole onFail and download stuff is IMO preemptive still.

I think npm is free to implement whatever parts of the spec it wants to support; they said they have no plans to support download, and I think that’s fine. I see no reason not to support onFail; npm wouldn’t support onFail: 'download', clearly, but there’s no reason not to support ignore, warn, error. That parallels the existing engines-strict where npm users can tell npm whether to error or warn on "engines" validation errors.

@wraithgar
Copy link

libc appears to be missing from the spec. It should live at the same level as os and cpu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests