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

Allow overruling constraint resolver #208

Open
GeoffreyBooth opened this Issue Oct 24, 2017 · 17 comments

Comments

Projects
None yet
3 participants
@GeoffreyBooth

GeoffreyBooth commented Oct 24, 2017

Migrated from: meteor/meteor#9244

@benjamn benjamn added this to the Meteor 1.6.1 milestone Oct 30, 2017

@theodorDiaconu

This comment has been minimized.

theodorDiaconu commented Dec 1, 2017

I believe there's gonna be a serious amount of work to support this. practicalmeteor:mocha was last updated a year ago.... wouldn't it better to just clone it locally inside packages, and just change the version number? Or publish it as a new one?

I think this constraint resolver will come with a price in building the app. And even if we do it with "opt-in" ability, someone should still modify the package to support this new "opt-in" ability ?

My concern is that this isn't a common problem. And having 2 versions of the same package loading is dangerous, for example, a package extending Mongo.Collection twice, may lead to very unexpected behavior.

@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Dec 1, 2017

Please read to the bottom of that thread. The suggested actions as of the end of the discussion were:

  • Allow installing a Meteor package from a Git repo, e.g. meteor add https://github.com/GeoffreyBooth/meteor-mocha.git . Atmosphere is getting polluted with forks of other packages, and allowing installing packages directly from GitHub would resolve this. (See https://atmospherejs.com/?q=t9n for an example.)
  • Give Atmosphere an equivalent of NPM’s https://www.npmjs.com/browse/depended, so we can see what packages have the most dependents.
  • In Atmosphere, provide a way to see all the dependents for a particular package, ordered by popularity. Currently a package’s Atmosphere page just shows the top few (see https://atmospherejs.com/meteor/coffeescript).
  • Provide a way to override the constraint resolver, like how it’s possible to do so with Bower. There are lots and lots of old packages out there that slap on api.versionsFrom with old versions of Meteor, needlessly constraining lots of dependencies. It be nice to be able to add something to .meteor/packages or .meteor/versions or package.json to override this constraint rather than needing to fork and patch the package.

The last item is the most important, and would actually resolve the problem; hence the new name of the issue. And it doesn’t involve allowing duplicate packages in the same project.

@theodorDiaconu

This comment has been minimized.

theodorDiaconu commented Dec 1, 2017

I've read it, but it seems I got a bit confused by the fact that Mocha had issues with newer coffeescript versions, and I did not see how the constraint overriding could fix the issue:
practicalmeteor/meteor-mocha#94

@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Dec 1, 2017

It’s not just that one package. See meteor/meteor#9107 (comment). I had this issue with nine packages just in the todos example app, which is hardly a complex app.

This issue will come up any time a commonly depended-upon package, like coffeescript or ecmascript, has a major version bump. ecmascript or babel-compiler probably should get a major version bump when they switch to Babel 7, and if there’s no way to override the constraint resolver, pretty much no one will be able to upgrade to the new ecmascript.

@theodorDiaconu

This comment has been minimized.

theodorDiaconu commented Dec 2, 2017

Thanks for the clarification.

@benjamn

This comment has been minimized.

Member

benjamn commented Dec 5, 2017

@theodorDiaconu While I fall somewhere in between you and @GeoffreyBooth in terms of how common I think this problem will be, I think it's important to be able to fix it reliably (if not easily) when it does happen, regardless of how common it is. In fact, the less common this situation is, the more we can treat the solution as something aimed at power-users, rather than something every Meteor developer needs to know how to do—and that makes designing the solution a little easier!

@benjamn

This comment has been minimized.

Member

benjamn commented Dec 5, 2017

How about this?

  • If the .meteor/packages file of an application has any exact @=x.y.z constraints, those versions take precedence over anything else, including any constraints that would normally be enforced by the constraint solver.
  • A non-fatal warning is printed when one of these exact constraints violates other constraints.

@benjamn benjamn self-assigned this Dec 5, 2017

@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Dec 5, 2017

Does it need to only be exact? What about @^x.y.z or @^x?

Either way, I’d like to still be able to run meteor update packagename to update it to the latest version, especially if it’s an exact version.

@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Feb 13, 2018

@benjamn Any word on this? It’s keeping me from being able to upgrade the coffeescript branch of the todos app—at least, without forking a bunch of packages:

=> Errors prevented startup:

   While selecting package versions:
   error: Conflict: Constraint coffeescript@1.0.4 is not satisfied by coffeescript 2.0.3_4.
   Constraints on package "coffeescript":
   * coffeescript@2.0.3_4 <- top level
   * coffeescript@1.0.4 <- arillo:flow-router-helpers 0.5.2
   * coffeescript@1.0.4 <- zimme:active-route 2.3.2
   * coffeescript@1.0.4 <- tap:i18n 1.8.2
   * coffeescript@1.0.4 <- softwarerero:accounts-t9n 1.3.11
   * coffeescript@1.0.3 <- practicalmeteor:chai 2.1.0_1
   * coffeescript@1.0.3 <- practicalmeteor:sinon 1.14.1_2

   Conflict: Constraint coffeescript@1.0.3 is not satisfied by coffeescript 2.0.3_4.
   Constraints on package "coffeescript":
   * coffeescript@2.0.3_4 <- top level
   * coffeescript@1.0.4 <- arillo:flow-router-helpers 0.5.2
   * coffeescript@1.0.4 <- zimme:active-route 2.3.2
   * coffeescript@1.0.4 <- tap:i18n 1.8.2
   * coffeescript@1.0.4 <- softwarerero:accounts-t9n 1.3.11
   * coffeescript@1.0.3 <- practicalmeteor:chai 2.1.0_1
   * coffeescript@1.0.3 <- practicalmeteor:sinon 1.14.1_2

=> Your application has errors. Waiting for file change.

In the short term at least, could we at least neuter the version constraint caused by api.versionsFrom? That’s the reason all of these packages implicitly imply coffeescript, even though they aren’t actually using the package, because coffeescript used to be a core package and now it’s not. If an explicit version number in the packages or versions file could override api.versionsFrom, that would at least solve the coffeescript case.

@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Jun 26, 2018

This has come up in the wild: https://stackoverflow.com/questions/50757646/meteor-1-6-coffeescript-reserved-word-import/50772818?noredirect=1#comment89003965_50772818

Any plans for a fix? Or at least any direction on what an acceptable fix would look like?

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 26, 2018

The last time I gave this some thought, I mostly convinced myself that we would need some sort of configuration (perhaps in the "meteor" section of package.json) to ignore specific version constraints of certain packages, such as

{
  "meteor": {
    "ignoreConstraints": {
      "practicalmeteor:chai": ["coffeescript", ...],
      ...
    }
  }
}
@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Jun 26, 2018

Seems wrong somehow to have some of the package version configuration in .meteor/packages, more in .meteor/versions, and yet more in package.json . . .

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 26, 2018

Another idea was to put it in .meteor/packages, as a JSON expression following a package name:

package1
package2
tap:i18n { "ignoreConstraints": ["coffeescript", ...] }
package3
...

though that requires inventing new syntax for that file.

@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Jun 26, 2018

Rather than finding all the packages that constrain coffeescript and adding configuration to tell them to ignore that constraint, couldn’t we add a flag to coffeescript to tell it to override constraints? Kind of like !important in CSS.

# .meteor/packages
coffeescript !important

Or whatever you want to call it.

The logic behind this is that if there can only be one installed version of coffeescript, what matters is whether or not that version can be constrained by other packages. Otherwise I need to keep adding ignoreConstraints for every package that I get a warning about, until I’ve made all the warnings go away.

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 29, 2018

@GeoffreyBooth What if api.versionsFrom could only set minimum versions for core packages, and otherwise would never hold back the versions of those packages? That seems like it would solve some of these problems, as long as we can determine whether or not a package version constraint came from api.versionsFrom (a question I will have to investigate).

@GeoffreyBooth

This comment has been minimized.

GeoffreyBooth commented Jun 29, 2018

That would solve this issue. It might be a little risky, in that some packages would get a much newer version than they’re expecting. Most packages aren’t really using most core modules, and few core modules have any breaking changes between major versions as far as I’m aware, but theoretically if a package is using a core module and expecting a certain version via versionFrom, and then that core module gets updated beyond a major version, the package could break.

But then again, I don’t know what better option we have. versionsFrom in its current form, locking down all core modules regardless of whether a particular package used that core module or needed a specific version, really ties our hands. Isn’t what you propose essentially a breaking change to versionsFrom?

Really this is an issue with any core (or formerly core) module. coffeescript is just a smaller case than something huge like ecmascript.

benjamn added a commit to meteor/meteor that referenced this issue Aug 15, 2018

Allow package-name@x.y.z! override syntax in .meteor/packages.
With this commit, if a top-level package version constraint in
.meteor/packages ends with a '!' character, any other (non-!) constraints
on that package elsewhere in the application will be weakened to accept
any version of the package that is not less than the constraint,
regardless of whether the major/minor versions actually match.

This functionality is extremely useful in cases where an unmaintained
package was last published with api.versionsFrom(<some ancient version>),
thus constraining the major version of any Meteor core package it depended
on, but you really want to upgrade that core package anyway. Just put a
'!' after the core package's version constraint in your .meteor/packages
file, and you will almost certainly get your way. The fact that minimum
versions are still enforced is good/fine because the constraints you want
to override are typically ancient, so they easily match any recent version
of the package.

Your only recourse before this @x.y.z! syntax was to find a replacement
for the unmaintained package, or fork and modify it locally, or somehow
persuade the package author to publish a new version with a more
reasonable api.versionsFrom. None of these options were easy.

Many thanks to @GeoffreyBooth, long-time maintainer of the `coffeescript`
package, for originally suggesting a ! syntax similar to this one:
meteor/meteor-feature-requests#208 (comment)

The limitation of this syntax to .meteor/packages is deliberate, since
overriding package version constraints is a power-tool that should be used
sparingly by application developers, and never abused by package authors.
Also, limiting the scope of this syntax reduces the risk of an arms race
between overrides, a la the infamous CSS !important modifier.
@benjamn

This comment has been minimized.

Member

benjamn commented Aug 15, 2018

@GeoffreyBooth ☝️😉

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