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

pnpm or generic package manager support #744

Closed
buschtoens opened this issue Aug 31, 2021 · 1 comment · Fixed by #787
Closed

pnpm or generic package manager support #744

buschtoens opened this issue Aug 31, 2021 · 1 comment · Fixed by #787

Comments

@buschtoens
Copy link

Currently ember-try only supports two package managers for installing node_modules:

  • npm: the default
  • yarn (v1.x): if the user opts-in by setting useYarn: true
  • (bower: technically supported in parallel, however it's deprecated since Deprecate bower support #402, basically dead 🧟, and doesn't actually manage node_modules.)

yarn v1.x is discontinued in favor of v3.x (dubbed berry). Unfortunately, the berry CLI and the new PnP package format is largely incompatible with ember-cli and ember-try. Related issues:

I doubt, that adopting berry will be easy or possible at all for the Ember ecosystem.

However by now, there are further actively maintained alternatives to choose from. For instance, there's:

  • pnpm: an alternative package manager, with a feature set similar to yarn, but better performance and disk efficiency.
  • rush: a monorepo (build) manager, similar to lerna, that wraps around the underlying package manager, currently supporting npm, pnpm & yarn.

I would love to start using pnpm for various reasons.

The main issue isn't the resulting node_modules structure, which luckily appears to be compatible with or eaasy to adapt to for ember-cli projects, at least in my limited experience thus far.
The deal breaker currently is the implicit ecosystem-wide assumption that either npm or yarn are being used.

There is an effort to fix that for ember install (ember-cli/ember-cli#9563), but npm / yarn are used explicitly in the wider ecosystem as well, like here in ember-try.

I got nerd-sniped and am happy to report that I was able to monkey-patch ember-try with the following snippet to make it use pnpm:

const NPMDependencyManagerAdapter = require('ember-try/lib/dependency-manager-adapters/npm');

// Maps arguments for `yarn` to their `pnpm` equivalents. A falsy value, like
// `undefined`, means that the argument is not supported by `pnpm` and will be
// dropped.
// https://github.com/ember-cli/ember-try/blob/v1.4.0/lib/dependency-manager-adapters/npm.js#L116-L123
const argsMap = {
  // > Don't read or generate a `yarn.lock` lockfile.
  // https://classic.yarnpkg.com/en/docs/cli/install/#toc-yarn-install-no-lockfile
  //
  // Normally we would enable `--frozen-lockfile`, however:
  // > If `true`, pnpm doesn't generate a lockfile and fails to install if the
  // > lockfile is out of sync with the manifest / an update is needed or no
  // > lockfile is present.
  // https://pnpm.io/cli/install#--frozen-lockfile
  //
  // Instead we use:
  // > When set to `false`, pnpm won't read or generate a `pnpm-lock.yaml` file.
  // https://pnpm.io/npmrc#lockfile
  '--no-lockfile': '--config.lockfile=false',

  // > Ignore engines check.
  // https://classic.yarnpkg.com/en/docs/cli/install/#toc-yarn-install-ignore-engines
  //
  // > During local development, pnpm will always fail with an error message, if
  // > its version does not match the one specified in the engines field.
  // >
  // > Unless the user has set the `engine-strict` config flag (see `.npmrc`),
  // > this field is advisory only and will only produce warnings when your
  // > package is installed as a dependency.
  // https://pnpm.io/package_json#engines
  //
  // ! This probably doesn't work as expected, because of the "during local
  // development" limitation.
  '--ignore-engines': '--config.engine-strict=false',
};

// Override the `init` method (= "`constructor`") of the `ember-try` dependency
// manager adapter for `npm`, so that `pnpm` support can be optionally enabled
// by setting `npmOptions: { manager: 'pnpm' }` in the `config/ember-try.js`.
//
// https://github.com/ember-cli/ember-try/blob/v1.4.0/lib/dependency-manager-adapters/npm.js#L13-L16
const originalInit = NPMDependencyManagerAdapter.prototype.init;
NPMDependencyManagerAdapter.prototype.init = function (...args) {
  originalInit.apply(this, args);

  // If `npmOptions.manager` is set to `"pnpm"`, then enable `pnpm` mode.
  //
  // `npmOptions` is passed in as `managerOptions`.
  // https://github.com/ember-cli/ember-try/blob/v1.4.0/lib/utils/dependency-manager-adapter-factory.js#L39
  if (this.managerOptions?.manager === 'pnpm') {
    // If `npmOptions` is an object with an `args` property, then let `args`
    // take the place of `managerOptions`, so that the upstream code can process
    // it.
    if (this.managerOptions.args instanceof Array)
      this.managerOptions = this.managerOptions.args;
    // If `npmOptions` itself already is an array (with an extra `manager`
    // property), then keep it as-is, so that the upstream code can process it.
    // If it isn't an array, and `args` is also not set, then unset
    // `managerOptions`, as `npmOptions` was only used to opt-in to `pnpm` mode.
    else if (!(this.managerOptions instanceof Array))
      this.managerOptions = undefined;

    // Enable the `yarn` code path, which is close to what we need for `pnpm`.
    this.useYarnCommand = true;

    // Substitute `yarn.lock` with `pnpm-lock.yaml` accordingly.
    this.yarnLock = 'pnpm-lock.yaml';

    // Note: the upstream convention is to append `.ember-try` _after_ the file
    // extension, however this breaks syntax highlighting, so I've chosen to
    // insert it right before the file extension.
    this.yarnLockBackupFileName = 'pnpm-lock.ember-try.yaml';

    // Patch the `run` method to replace `yarn` with `pnpm` and translate &
    // filter the arguments accordingly.
    const originalRun = this.run;
    this.run = (_cmd, args, opts) =>
      originalRun.call(
        this,
        'pnpm',
        args
          .map((arg) => (arg in argsMap ? argsMap[arg] : arg))
          .filter(Boolean),
        opts
      );
  }
};

I keep this file in an extra directory that is .npmignore'd and import it in config/ember-try.js like so:

const getChannelURL = require('ember-source-channel-url');
const { embroiderSafe, embroiderOptimized } = require('@embroider/test-setup');

try {
  require('../patches/ember-try');
} catch {
  console.warn('Failed to patch `ember-try` for `pnpm` support.');
}

module.exports = async function () {
  return {
    npmOptions: {
      manager: 'pnpm',
      args: ['--prefer-offline'],
    },

    scenarios: [ /* ... */ ]
  }
};

It's not perfect, but it works. 🥳

I would be more than happy to PR this, so that ember-try can officially / experimentally support pnpm. The necessary underlying code changes aren't too drastic, but I'm not entirely sure regarding:

  • What the official user-facing API should look like; useYarn + usePnpm doesn't really make sense and means that we'll keep having this issue, when the next package manager / wrapper comes along.
  • Whether this might be a good opportunity to refactor the internal API, so that it becomes more flexible and might even allow users to provide their own adapters.

What are your thoughts on this?

@kategengler
Copy link
Member

I'm not sure if pnpm support across the board should be an RFC.

If we do implement in ember-try, I think the the thing to do would be to create a PnpmDependencyManagerAdapter, as the intended pattern was for there to be an adapter per package manager. If there's heavy overlap we can extract some common parts to utilities.

I know in general there have been some plans to allow overriding of install, etc in the config, and that would also be an alternative.

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

Successfully merging a pull request may close this issue.

2 participants