jspm / registry

The jspm registry and package.json override service
https://jspm.io
229 stars 255 forks source link

Caught out by 'missing' override #556

Open myitcv opened 9 years ago

myitcv commented 9 years ago

I think this has been raised in various places and in various guises before so apologies if this is noise repeated elsewhere (I couldn't find anything on this particular suggestion)

The package in question is bootstrap-slider [Github]

As you can see the registry contains an override for v4.5.4

The current version is v5.0.13

Looking at the output of jspm install bootstrap-slider:

$ jspm --version
0.16.2
$ jspm install bootstrap-slider
     Creating registry cache...
     Looking up github:seiyria/bootstrap-slider
ok   Installed bootstrap-slider as github:seiyria/bootstrap-slider@^5.0.13 (5.0.13)
ok   Install tree has no forks.

warn core-js@0.9.18 is unsupported for this version of jspm. Use jspm dl-loader to update.

ok   Install complete.

(I'll ignore the error about core-js for now)

Clearly no override has been installed here. Nor is there any indication that one might be needed.

Can I suggest that the install should have failed at this point, because there exists an override for another (not necessarily earlier) version of the same package?

The thinking being: if an override were needed for another version, it is highly likely one is needed for this version too.

The solution for this particular package (as I understand it from here) would either be:

Would have the added benefit of improving the quality of the registry.

Have I missed something obvious here? Thoughts?

As a side note, is the registry 'tested' in any way, i.e. how do we know at a given point in time that the provided overrides 'work'?

Thanks

guybedford commented 9 years ago

The idea is that overrides are only ever temporary - the place where overrides truly belong is in the package.json of the original package, managed by the owner. In a world where everyone uses jspm, overrides shouldn't be necessary.

So this workflow is designed to transition towards such a scenario. The existence of warnings when installing is usually a good sign that the package needs an override. Perhaps we just need to document this, and also encourage workflows around pushing overrides to the original repos. There were also plans of making this all automated via a PR bot, but that issue is still outstanding.

Let me know if that makes sense to you, or if you have any suggestions here as well.

myitcv commented 9 years ago

@guybedford thanks for the response.

The idea is that overrides are only ever temporary - the place where overrides truly belong is in the package.json of the original package, managed by the owner. In a world where everyone uses jspm, overrides shouldn't be necessary

100% with this, but I think we can improve the experience of those who find themselves installing packages that are in the transition period (for whatever reason)

The existence of warnings when installing is usually a good sign that the package needs an override.

Just to be clear, those warnings that I commented on above are unrelated (as far as I can tell) to bootstrap-slider. In a fresh jspm-init-ed project, there are no warnings:

$ npm init
...
$ npm install --save-dev jspm
...
$ export PATH=$PWD/node_modules/.bin:$PATH
...
$ jspm init
Would you like jspm to prefix the jspm package.json properties under jspm? [yes]:
Enter server baseURL (public folder path) [./]:
Enter jspm packages folder [./jspm_packages]:
Enter config file path [./config.js]:
Configuration file config.js doesn't exist, create it? [yes]:
Enter client baseURL (public folder URL) [/]:
Do you wish to use an ES6 transpiler? [yes]:no
ok   Verified package.json at package.json
     Verified config file at config.js
     Looking up loader files...
       system.js
       system.src.js
       system.js.map
       system-csp-production.js
       system-csp-production.src.js
       system-csp-production.js.map
       system-polyfills.js
       system-polyfills.src.js
       system-polyfills.js.map

     Using loader versions:
       systemjs@0.18.17
ok   Loader files downloaded successfully

$ jspm install bootstrap-slider
     Creating registry cache...
     Looking up github:seiyria/bootstrap-slider
ok   Installed bootstrap-slider as github:seiyria/bootstrap-slider@^5.0.13 (5.0.13)
ok   Install tree has no forks.

ok   Install complete.

My understanding of the jspm install flow is a bit vague so perhaps you can sense check this suggestion:

  1. If there is a package.json override in the original package use it and stop
  2. If there exists an override in the JSPM registry for this package (any version):
    1. If there is a match according to the rules of the semver matching (as described in the quoted text here) use it and stop
    2. Else exit with an error and stop, warning that there probably should be an override for the version being install but isn't
  3. Else install according to the existing rules and only exit with an error if there is a problem

My suggestion here is the introduction of step 2.ii. At the moment this step does not exist and so my install of bootstrap-slider falls through to case 3, and seemingly succeeds.

I can't help thinking I'm probably not alone in this situation... Anyone installing bootstrap-slider via JSPM since Mar 14 (when bootstrap-slider v4.5.5 was released) will likely have hit this problem and either:

  1. Worked around it with a local override
  2. Not realised there's an issue and given up

Or perhaps I'm overstating the impact?

guybedford commented 9 years ago

We can't fail in this case, because the author may now be supporting jspm as of the new major and it would be perfectly configured to work out without an override being necessary. Failing an install isn't really acceptable as we never know if something might not work until the user tries it. And that is the pain because the path to make it work isn't obvious.

There are a number of efforts that can be made here, but all require a team effort (wish I was wealthy for these kinds of things):

Other suggestions always welcome, it's a pain point for sure.

myitcv commented 9 years ago

We can't fail in this case, because the author may now be supporting jspm as of the new major and it would be perfectly configured to work out without an override being necessary.

How is it determined that a package supports jspm?

guybedford commented 9 years ago

A package is considered jspm aware if it has a registry or jspm property in its package.json. But even further than that, consider a standard install from GitHub or npm - most packages installed aren't jspm-aware but will work in jspm. A major version can change structure and may well be as different as another package - it might work fine even without any jspm knowledge in the same way as any package, and the override may likely be no longer necessary or completely out of date.

myitcv commented 9 years ago

Thanks.

Just so that I'm clear, the problem only arises for packages which have at some point in their history required an override that has been placed in the JSPM registry. If a package has never required an override, then case 2.ii in my earlier post will never get hit.

To further sub categorise these problem packages into (apologies for the number overloading!):

  1. Packages which now have an override defined in their package.json, thereby taking precedence over what is in the JSPM registry (by definition these are JSPM aware, correct?)
  2. Packages which no longer need an override, through some code refactor or some such, a group which can be further subcategorised into:
    1. Those which are not jspm aware according to your definition
    2. Those which are jspm aware according to your definition

Where a package is JSPM aware, then I think it's fair to assume the package owners have assumed responsibility for their package 'working' via JSPM. This covers cases 1 and 2.ii in the list above.

Which leaves category 2.i. Is this not covered by an 'empty' override being added to the JSPM registry?

Failing an install isn't really acceptable as we never know if something might not work until the user tries it.

Except, as I mentioned above, if the package is jspm aware, in which case they (the package owner) is saying "this works via JSPM" and should probably be held to it. Again, if a package is not jspm aware and has never required an override, current behaviour remains.

In case a package is jspm aware at the version being installed, then the package.json for that package should be consulted for any overrides. If there are some, use them, if not, we're done.

For those packages which are not jspm aware then case 2.ii from my earlier post would help people falling foul of packages which still require an override (but where an override for their version is 'missing' from the registry), with the 'empty override' solution above being an option for those packages which no longer require an override as of version X (same semver matching rules would apply)

Thoughts?

guybedford commented 9 years ago

The empty override was about nulling out the override within the semver compatibility range - a package works for 1.1.1, then they add jspm support in the package.json for 1.2.0, so we add an empty override so >1.2 now all just use the package.json override. Empty overrides were never about nulling out packages for version 2 outside of the semver compatibility range.

My point is that version 2 is as good as being an entirely new package as far as we're concerned, without other information to indicate otherwise. In addition, as said, the focus should be on ensuring case 2i happens rarely, by making sure that the version 2 of the package is jspm-configured. When 2i does happen though, it is entirely equivalent to the standard override problem for installing any package that is not jspm configured.

myitcv commented 9 years ago

When 2i does happen though, it is entirely equivalent to the standard override problem for installing any package that is not jspm configured.

Ok, that I understand.

bootstrap-slider however is a package that still requires an override and the package is still not jspm aware. There was an override put in the registry for v4.5.4 (back in March) but nothing was pushed to the original package repo to make it jspm aware. This original override was specific to v4.5.4.

Given my earlier comment, using jspm install for each of the 20 odd versions that followed will have 'worked' (the install will have succeeded without errors) but things will have failed at runtime, which is a hard problem to diagnose. But for each of these versions we know:

My suggestion is that given these two facts, we should either:

In either situation the warning/error message can direct the user towards some documentation that suggests they either:

My worry is that in doing nothing we leave many developers to fall into the trap that I did when we have sufficient information to at least warn that something might not be right.

guybedford commented 8 years ago

Sure, I get what you mean here. Warning when we can warn is certainly a good strategy.