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

[bugfix] Allow VersionChecker to work with Projects #51

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Apr 21, 2018

There are two main use cases for VersionChecker today:

  1. Checking the version of an NPM dependency of an Addon
  2. Checking the version of an NPM or Bower dependency of the root Project

These use cases are currently intermingled in the VersionChecker API, which causes some confusing behavior:

  1. When passing an Addon to a VersionChecker:

    • Checking an NPM dependency will check the addon's package.json directly, but checking a Bower dependency will check the root Project's bower.json. There is no way to check the root Project's NPM dependencies.

    • Attempting to use forEmber will attempt to check the Addon's version of ember-source (which shouldn't exist), and if it is not found will attempt to check the Bower dependencies of the root Project. This forking of behavior is confusing because it correctly detects the version of Ember for users who are still using Bower, but will not for users who switch to ember-source.

      The "fix" for this is to use _findHost to find the root EmberApp and to pass that to VersionChecker instead, but this breaks as described below and is still confusing (why can I even call forEmber on a VersionChecker for a parent addon when it has this behavior?)

  2. When passing a Project to a VersionChecker:

    • It will error because the Bower checker assumes that it will receive an item with a project property which is a Project.
  3. When passing an EmberApp to a VersionChecker:

    • It will attempt to check NPM dependencies using an undefined root, but will correctly check Bower dependencies. This generally works, but breaks when using yarn link because NPM's module resolution defaults to the addon which is using VersionChecker rather than the Project root.

This PR proposes that we should clarify the API by enforcing that users explicitly pass the thing they are trying to check the version of, either an Addon or a root Project, to VersionChecker:

included(parent) {
  this._super.included.apply(this, arguments);

  // Currently the only meaningful way to check if the parent is an Addon or an EmberApp
  if (parent.root !== undefined) {
    let parentAddonChecker = new VersionChecker(parent);
  
    parentAddonChecker.forEmber(); // Should throw an error, doesn't make sense
    parentAddonChecker.for('foo', 'bower'); // Should throw an error, doesn't make sense
    parentAddonChecker.for('foo', 'npm'); // allowed
  }

  let rootVersionChecker = new VersionChecker(this.project);

  parentAddonChecker.forEmber(); // allowed
  parentAddonChecker.for('foo', 'bower'); // allowed
  parentAddonChecker.for('foo', 'npm'); // allowed
}

We would do this by first allowing VersionChecker to accept a Project directly (as this PR does), adding deprecations, and allowing users to switch over to the new usage style, then creating a new major version.

VersionChecker currently can receive either an Addon or
EmberApp/EmberAddon to check dependencies against. If the user is
checking for NPM dependencies, then VersionChecker will attempt to
resolve the `package.json` of the Addon/App with a
`basedir = (app || addon).root`. Addon's know their root directory, but
EmberApp's do not - it is stored on their `project`. So, when the NPM
checker goes to resolve `package.json` of an EmberApp, it always
resolves with `undefined`, which defaults to the current NPM project.

In most cases this works just fine. However, it breaks whenever you do
`npm link`, which can cause heisenbugs while working in local projects
and generally is not fun. Additionally this same problem is causing
issues in `lerna` based projects, as they symlink the packages of the
projects together rather than install the standard way.

The ideal solution would be to change the API to only allow users to
provide an `Addon` or a `Project`, and force users to distinguish
between when they want dependencies to check of the base app and when
they want to check dependencies of a parent addon. For instance,
checking the Ember version _only_ makes sense when checking it against
the root application, so users should always do
`new VersionChecker(addon.project).forEmber()`. This would be more
verbose in some cases, but would prevent this ambiguity.

The short solution proposed by this PR moves us incrementally in that
direction by allowing the `VersionChecker` to receive a Project in the
first place, by checking the addon to see if it has a `project` field
first (as in the case of EmberApp/Addon) or if it has `root` and
`bowerDirectory` itself, throwing if neither is fulfilled.
@pzuraq pzuraq force-pushed the bugfix/allow-checking-against-projects-directly branch from 9067b36 to 27b6318 Compare April 21, 2018 16:51
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

The description infers this affects npm and bower alike, but I only see changes to the bower dependency checker. Am I misunderstanding something or is there a missing change?

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 21, 2018

So, the real blocking issue here is that the Bower dependency checker always assumes there will be a project property on whatever we pass to VersionChecker. So if I pass a Project directly, it'll throw an error whenever we're using forEmber and it doesn't find ember in the NPM deps (or whenever we attempt to find the Bower deps).

However, if we pass an EmberApp, it doesn't have a root property, so the NPM checker doesn't check the correct directory. It's a catch 22 🙃

In order to correctly check an Ember app's Ember version today, you would have to do something like this:

included() {
  let project = this.project;

  let checker = new VersionChecker({ root: project.root, project });
}

@rwjblue rwjblue merged commit 80f6d74 into ember-cli:master Apr 27, 2018
@rwjblue rwjblue added the bug label Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants