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

Recommendation to install beta breaks ecosystem #2260

Open
BryanCrotaz opened this issue Jan 17, 2021 · 28 comments
Open

Recommendation to install beta breaks ecosystem #2260

BryanCrotaz opened this issue Jan 17, 2021 · 28 comments

Comments

@BryanCrotaz
Copy link
Contributor

From the docs:

Installing the library is as easy as:
ember install [email protected]

However this beta is not marked as latest, so any other addons that require ESA won't work - Babel will use random parts of the two libraries.

For example, using ember-simple-auth-token for JWT auth:

yarn why ember-simple-auth
yarn why v1.22.10
[1/4] Why do we have the module "ember-simple-auth"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "[email protected]"
info Has been hoisted to "ember-simple-auth"
info This module exists because it's specified in "devDependencies".
info Disk size without dependencies: "1.8MB"
info Disk size with unique dependencies: "2.8MB"
info Disk size with transitive dependencies: "52.19MB"
info Number of shared dependencies: 236
=> Found "ember-simple-auth-token#[email protected]"
info This module exists because "ember-simple-auth-token" depends on it.
info Disk size without dependencies: "2.73MB"
info Disk size with unique dependencies: "3.73MB"
info Disk size with transitive dependencies: "53.12MB"
info Number of shared dependencies: 236

In my case this means that

beforeModel(transition: any) {
    this.session.requireAuthentication(transition, 'login');
}

throws - session doesn't have any methods, only the authenticated data block. Uninstalling ember-simple-auth-token makes it work again.

Please can we get a 3.1.0 release, and never recommend installation of a beta - that's guaranteed to cause these types of problems.

@BryanCrotaz
Copy link
Contributor Author

@fenichelar
Copy link

However this beta is not marked as latest, so any other addons that require ESA won't work

You can use ember-simple-auth-token with versions of ember-simple-auth that are not marked as latest. In the vast majority of the applications I manage, we use an older version of ember-simple-auth and the latest version of ember-simple-auth-token. You simply ember install the specific version of ember-simple-auth that you want to be used.

and never recommend installation of a beta - that's guaranteed to cause these types of problems.

It should not cause problems. You are implying that you can only use the latest version of a package that another package depends on which is not true. You can use more recent version or older versions. It just has to be in the range of versions that satisfy the dependency.

If package A depends on package B ^2.1.0 then you use any version of package B that is greater or equal to 2.1 and less than 3.0.

ember-simple-auth-token has the following in package.json

"ember-simple-auth": "^1.6.0 || ^2.0.0 || ^3.0.0"

[email protected] satisfies this dependency just like [email protected] does even though neither are marked as latest.

@BryanCrotaz
Copy link
Contributor Author

You are misunderstanding.

[email protected] is not backwards compatible. It implements parts of the documented spec that are not implemented in 3.0.1. e.g. this.session.requireAuthentication

An app written for 3.1.0-beta.1 will break in really weird ways if 3.0.1 is installed instead.

ember-simple-auth-token is calling for the latest 3.x version. Which is 3.0.1, NOT 3.1.0-beta.1. And Babel does not allow two addons of different versions. In this case, 3.0.1 is winning.

There is nothing wrong with ember-simple-auth-token. The problem is here in ember-simple-auth, using a beta version as the recommended release.

@BryanCrotaz
Copy link
Contributor Author

BryanCrotaz commented Jan 17, 2021

If package A depends on package B ^2.1.0 then you use any version of package B that is greater or equal to 2.1 and less than 3.0

This would only be true if package B correctly uses Semver, or the version required was in the major.minor.patch chain and was not as in this case a beta.

@fenichelar
Copy link

ember-simple-auth-token is calling for the latest 3.x version.

It is not calling for the latest version of 3.x. It is calling for any version of 3.x that matches ^3.0.0.

This would only be true if package B correctly uses Semver, or the version required was in the major.minor.patch chain and was not as in this case a beta.

Yes. I was not aware that 3.1.0-beta.1 did not match ^3.0.0.

const semver = require('semver');
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0'); // => false
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0', { includePrerelease: true }); // => true
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0-alpha'); // => false
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0 || ^3.1.0-alpha'); // => true

@BryanCrotaz
Copy link
Contributor Author

As the recommendation in readme is to install the beta, this inherently makes the beta a release, no? Can we not just make a 3.1.0 release?

@marcoow
Copy link
Member

marcoow commented Jan 18, 2021

An app written for 3.1.0-beta.1 will break in really weird ways if 3.0.1 is installed instead.

Of course it will ;) 3.1.0 introduces new APIs that do not exist in 3.0.1. That does not mean 3.1.0 is not backwards compatible.

We're aiming for a stable 3.1.0 within the next few weeks.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

@BryanCrotaz I would strongly recommend to use https://github.com/salsify/ember-cli-dependency-lint to avoid these kinds of issues in the future, and if you use yarn you can use https://github.com/atlassian/yarn-deduplicate to deduplicate dependencies in your lockfile to solve the version conflicts.

@BryanCrotaz
Copy link
Contributor Author

:) I already use that - that's how I worked out what the problem was! I've used yarn's resolutions to fix it. The underlying problem is that a beta version is the recommended install in the readme, and so any other addons that extend ESA will break the developer's app.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

the beta is only recommended in the README of the master branch, which corresponds to the beta release. if you look at the README for the stable version you shouldn't find such a recommendation AFAIK.

so any other addons that extend ESA will break the developer's app.

that's only true if you don't use dependency-lint or are not generally aware of the issue. and I do agree that it is an issue, but it is a more general issue, that is not just limited to ESA. even if there was a v3.1.0 release here your issue would not be resolved because the lockfile would still point to v3.0.x for existing installs.

@fenichelar
Copy link

When using ember-simple-auth-token, you should not be explicitly installing ember-simple-auth.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

When using ember-simple-auth-token, you should not be explicitly installing ember-simple-auth.

this is starting a somewhat different discussion now, but: why not?

@fenichelar
Copy link

ember-simple-auth-token depends on ember-simple-auth. A compatible version of ember-simple-auth is automatically installed when you install ember-simple-auth-token.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

sure, but that does not mean that you can't install ESA explicitly too. you only have to be careful that both versions match up. the alternative would be that ember-simple-auth-token would declare ESA as a peer dependency instead.

@fenichelar
Copy link

@Turbo87 Yes, you are correct. You can explicitly install ESA if you want but you need to make sure to install a supported version.

@BryanCrotaz
Copy link
Contributor Author

I have submitted a PR to esa-token that makes ESA a peerDependency.

Please, one of you agree to change your stance. The two addons are at loggerheads, you disagree, and you both say the other is wrong. All I can do as an app developer is watch and feel sad. And fork my own solution, which is not in the ethos of the Ember community.

@fenichelar
Copy link

Please, one of you agree to change your stance. The two addons are at loggerheads, you disagree, and you both say the other is wrong

What exactly do we disagree on? I don't recall saying ESA was doing anything wrong. I said you were doing something wrong by trying to use a version of ESA that ember-simple-auth-token does not support. I don't see a need for any changes on ESA or ember-simple-auth-token.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

Yes. I was not aware that 3.1.0-beta.1 did not match ^3.0.0.

FWIW https://jubianchi.github.io/semver-check/#/%5E3.0.0/3.1.0-beta.1%20 disagrees with that statement 🤔

@fenichelar
Copy link

@Turbo87

FWIW https://jubianchi.github.io/semver-check/#/%5E3.0.0/3.1.0-beta.1%20 disagrees with that statement

const semver = require('semver');
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0'); // => false
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0', { includePrerelease: true }); // => true
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0-alpha'); // => false
semver.satisfies('3.1.0-beta.1', '^1.6.0 || ^2.0.0 || ^3.0.0 || ^3.1.0-alpha'); // => true

I would assume that means semver-check is setting includePrerelease to true.

@BryanCrotaz
Copy link
Contributor Author

BryanCrotaz commented Jan 18, 2021

Yes. I was not aware that 3.1.0-beta.1 did not match ^3.0.0.

FWIW https://jubianchi.github.io/semver-check/#/%5E3.0.0/3.1.0-beta.1%20 disagrees with that statement 🤔

And yarn install disagrees with that website.

In fact it has to - otherwise any time you put out an alpha release you'd break everyone out there.

@BryanCrotaz
Copy link
Contributor Author

What exactly do we disagree on? I don't recall saying ESA was doing anything wrong. I said you were doing something wrong by trying to use a version of ESA that ember-simple-auth-token does not support. I don't see a need for any changes on ESA or ember-simple-auth-token.

  • ESA is saying you should have ESA as a peerDependency, and the user should install both addons
  • You are saying that it's wrong to have ESA as a peerDependency, and the user should not install both addons

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

ESA is saying you should have ESA as a peerDependency, and the user should install both addons

I never said that, and I am not ESA. What I said was that it is one alternative possibility.

@fenichelar
Copy link

You are saying that it's wrong to have ESA as a peerDependency, and the user should not install both addons

I didn't say this was wrong. I said that based on the fact that ESA has historically not been a peer dependency of ember-simple-auth-token, I'm not sure it makes sense to change it

@BryanCrotaz
Copy link
Contributor Author

OK, this is getting silly.

The situation is that anyone following the instructions will end up with a broken app and obscure error messages.

Between these two addons you need to sort out what you're going to do. I've submitted a PR to ember-simple-auth-token which fixes the problem. If you don't like it, come up with something else.

But don't sit there denying there's a problem and saying it doesn't make sense to change anything.

I'm off to do some work on my own code. I'll have to reimplement ESA as it's just been too much hassle coping with the upgrades.

@fenichelar
Copy link

@BryanCrotaz I will add a note to the ember-simple-auth-token readme that says that you should let ember-simple-auth-token install ESA or ensure that you are using a version of ESA that is compatible with ember-simple-auth-token,

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

@fenichelar it looks like you're right. yarn-deduplicate does indeed not deduplicate the ESA-token dependency and the explicitly installed ESA beta version.

I guess the only way to resolve this then is using resolutions. I'm not sure if moving to a peer dependency would really fix anything because the version constraints would still be the same. I know that yarn only outputs warnings for broken peer dep constraints though, so that might work, but npm has roughly 4 different ways of dealing with peer deps, so no idea how the situation there works 😅

@fenichelar
Copy link

@Turbo87 What is the recommended version of ESA to use in production at this time?

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 18, 2021

I would assume the latest stable release, but I'm not the maintainer of this addon, so I can't say for sure 😅

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

No branches or pull requests

4 participants