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

Splitting Ember into packages #284

Closed
wants to merge 4 commits into from
Closed

Splitting Ember into packages #284

wants to merge 4 commits into from

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Dec 13, 2017

@knownasilya
Copy link
Contributor

Beautiful!

@Kerrick
Copy link

Kerrick commented Dec 13, 2017

It's unclear to me from the RFC, but will Semantic Versioning be respected in @ember/core?

Given the given example from the RFC, let's assume Ember 3.2.0 is released and it splits out @ember/partial and @ember/prototype-extensions into their own packages. That means @ember/core no longer has the same API surface area as it did before, so it is a breaking change. Does that mean that @ember/core's major version number will have to be bumped? If so, that means the @ember/core version number will quickly fall out-of-sync with the ember-source version number. If not, that means @ember/core would not follow semver.

What are the trade-offs between those, and which is the current plan?

@cibernox
Copy link
Contributor

cibernox commented Dec 13, 2017

I find some are unclear for addon authors.
Today is common and a good practice to test your addons in beta and canary versions of Ember. The Scenario: Addon author declaring dependencies doesn't clarify enough how the required-base-features will work when testing with a version beyond stable.
I have the feeling that if we don't handle that gracefully, people who want to ride the beta-wave (not to mention canary) will have to either use ember-source to be safe, or every time they update to a new beta, as the ecosystem will naturally lag behind, they have to heavily edit the config/dependency-overrides.js to satisfy all the unmet new packages.

This RFC is pretty dense, so I might just as well be misunderstanding something.

@Panman82
Copy link

Am I in the minority of wanting one "monolithic" ember package? I'd rather have everything "ember" at my disposal, then have tree-shaking remove the un-needed stuff on build (shake not only my app code, but what pieces of ember are not being used by the app and addons, etc.) Just seems like this would add so much more complexity when tree-shaking could take care of the job. Or am I missing something?

@rtablada
Copy link
Contributor

@Panman8201 that is achieved here. If you want "everything ember" you'll continue to use ember-source if you want to explicitly declare your dependencies (or dare I say add Ember features to a Glimmer app in the future) then you can include only the bits you want.

@chancancode
Copy link
Member

@Panman8201 I think you are definitely in the majority. I think most users wouldn't have to worry about this at all, and as long as they continue to use ember-soure (maybe rename to something like @ember/all in the future) things will work exactly the way they are today. That should still be the default in guides, CLI blueprint etc.

You only need to do this if you are very filesize-conscious (I personally think most apps – at least most "productivity" kind of desktop apps aren't in that category). But it's nice that people in that category can do it if they want to.

@chancancode
Copy link
Member

Also, I don't think tree shaking can be very effective at removing the kind of features we are talking about here. For example, Mixins, computed properties and observers are all features that are deeply weaved into different parts of the framework. Even if you are not importing the macros in your app the Ember.Object (for example) will have to handle all these possibilities in the constructor, so these things still ended up being imported, so from the bundler's perspective they are all "used". Template features like partials are also very difficult to tree-shake, since you don't even have an import statement.

Not that it's impossible, but the bundler would have to get really smart and have a lot of knowledge about Ember's internals to do it correctly.

@chancancode
Copy link
Member

@Kerrick

Critically, @ember/core includes an extra rule in its semver policy: the only supported way to upgrade to a newer @ember/core is to run the ember-cli-update command. The update command will be aware of any additional packages have been split off from @ember/core since your last version, and it will add them automatically to your app.

When you run the upgrade command, it will edit your package.json such that the newly split packages are included in your package.json by default, so it behaves exactly the same way after the upgrade.

@Kerrick
Copy link

Kerrick commented Dec 13, 2017

When you run the upgrade command, it will edit your package.json such that the newly split packages are included in your package.json by default, so it behaves exactly the same way after the upgrade.

@chancancode I understand that the proposal includes an upgrade command to help manage breaking changes in @ember/core that result from feature extraction, but will that tool be upgrading the @ember/core package's major or minor version?

A potential problem with this being a minor version bump to @ember/core is that traditional semver does not consider the use of external tools to maintain compatibility -- it only considers version numbers. So this could introduce compatibility issues with other external tools that rely on semver.

@chancancode
Copy link
Member

@cibernox that is the point of the "required-core-features" system. It is basically saying "this addon was last audited as of version 3.10" – so if version 3.11 splits out two additional packages (say @ember/computed-properties and @ember/observers), then we will conservatively assume the addon requires those features by default.

So everything will still work, but your build might not be as lean as your hoped (it will be no worse than before the upgrade, though). You can use the override feature to test if anything breaks if you insist to remove those features and send a PR to the addon to "re-certify" it against 3.11.

@knownasilya
Copy link
Contributor

knownasilya commented Dec 13, 2017

@Kerrick at the moment ember-cli-update has from and to flags, for specifying versions, so I'm assuming those will stay or additional flags would be added as shortcuts, e.g. --major, --minor, or --patch.

@chancancode
Copy link
Member

The problem with this is that semver does not consider the use of external tools to maintain compatibility -- it only considers version numbers.

@Kerrick I don't think that is true.

The only relevant concept in Semver is "public API", which is intentionally very loosely defined:

Software using Semantic Versioning MUST declare a public API. This API could be declared in the code itself or exist strictly in documentation.

As part of the documentation, this piece of software communicates to the users that "the only supported way to upgrade is to use the upgrade command", which does whatever it needs to do to maintain the API compatibility for you.

It is also absolutely not true that you don't have to use "external tools" to maintain compatibility. In practice all node packages have the implicit requirement of "the only supported way to upgrade is to use an NPM-compatible package manager" (where "NPM-compatible package manger" is not even that well defined).

If you try to upgrade a package simply by downloading the tarball of a newer minor version and extract that into the appropriate location in your node_modules, it will likely not work because you will be missing some newly required dependency.

It is just a matter of clearly defining what you consider the "public API" and communicating that clearly with your users, which is why even things like legal documents could be considered "semver-compatible".


We propose adding an *additional* supported way to depend on Ember. You can choose to run `ember explode`, and we will remove `ember-source` from your dependencies and replace it with `@ember/core` *plus the current complete set of already-extracted Ember packages*. `ember explode` is intended to not change the set of Ember features in your app. It just splats them out into a form that comes from separate packages.

Critically, `@ember/core` includes an extra rule in its semver policy: the only supported way to upgrade to a newer `@ember/core` is to run the `ember-cli-update` command. The update command will be aware of any additional packages have been split off from `@ember/core` since your last version, and it will add them automatically to your app.
Copy link
Member

Choose a reason for hiding this comment

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

until now ember-cli-update is responsible for updating ember-cli, I fear that this change force ember-cli even more into lockstep with ember-source 🤔


If one of the newly-split-off Ember packages depends on another, it will say so in its NPM peerDependencies. We will use exact-version constraints, which is effectively how things already work today.

NPM has historically been loose about peerDependencies, so they are often ignored by developers. We propose that ember-cli should hard error for missing peerDependencies to avoid this problem (most people who think they don't really need to clean up peerDependency warnings are simply mistaken, and have latent bugs).
Copy link
Member

Choose a reason for hiding this comment

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

we have discussed this in the CLI calls and I am very much against a hard error without any escape valves

Copy link
Member

Choose a reason for hiding this comment

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

That is the only part that jumped out at me too. Hard errors might cause more pain than what it would be trying to solve. At least once I got peer deps unsatisfied warnings from NPM, where I knew my setup was more correct than NPM's algorithm. I think we should roll this out without hard errors, then bring that discussion up when it becomes a problem.

"@ember/component"
]
}
};
Copy link
Member

Choose a reason for hiding this comment

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

how does this format address overriding incompatible peer deps?


In the case of code we want to remove, splitting it into separate packages doesn't eliminate our need to support it (at least until the next major release lets it be truly removed). So while this proposal lets us "go faster" in some senses, it doesn't let us reduce support any faster.

# Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if static (import) analysis to figure out what Ember features are needed would also be a valid option instead of declaring the features explicitly 🤔

@Turbo87
Copy link
Member

Turbo87 commented Dec 13, 2017

and FWIW I tend to agree with @Kerrick that @ember/core would very likely not follow semver if implemented in lockstep with ember-source

@simonihmig
Copy link
Contributor

What @chancancode said in #284 (comment) was quite helpful to me to clarify the difference to only relying on tree-shaking. Maybe something like this could be added as another "How this relates to" section?

Copy link
Contributor

@Gaurav0 Gaurav0 left a comment

Choose a reason for hiding this comment

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

Overall, I am lukewarm to having all this new configuration. I would strongly prefer to look for ways not to import all of Ember automatically, like tree shaking, static import analysis, etc.


# How We Teach This

The most important message we need to teach app developers is: use `ember-cli-update` whenever you're changing your Ember version. As long as we can spread that message, we can provide direct guidance the rest of the way. For example, when we reach a sufficient level of confidence in the `@ember/core` packaging, we can begin making `ember-cli-update` offer to automatically explode `ember-source` into `@ember/core` *et al*. This would be a simple code mod that should not alter any app semantics.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should ember-cli-update be added to the default blueprint? I think it would make it easier to set up and teach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, whatever upgrade command we use should be in the default blueprint. It doesn't necessarily even need to be ember-cli-update, it could be something new. I just picked that one because it's the best one that already exists.

Choose a reason for hiding this comment

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

Keeping "npm-SemVer" would lessen the strain here. You would still have to teach people to cope with frequent major versions by using the provided magic. But you wouldn't lose people along the way that you haven't reached yet.

"required-core-features": "3.1.0",
"required-additional-features": ["@ember/routing", "@ember/prototype-etxensions", "@ember/string"]
}
```
Copy link
Contributor

@Gaurav0 Gaurav0 Dec 14, 2017

Choose a reason for hiding this comment

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

I'm really concerned this puts additional burdens on addon developers, who already have to do a lot. Will this be expected of most/all ember addons? If a single addon doesn't do this, it will be assumed that all of ember is required? Will addons be continually pressured to update "required-core-features" with every release of Ember?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an area of legitimate concern, and one I thought about a lot when writing this RFC. Here is my reasoning.

My goal here is to enable people who choose to spend effort on getting code out of their apps to be unblocked from doing so, and that people who don't choose to spend effort on the whole thing should not be impacted. For app authors, I think the proposal clearly achieves that goal.

For addon authors, there is this one-line change in package.json that I am indeed requiring, if they want to make it easy for some of their advanced users to start dropping Ember packages. To make that one-line change as easy as possible, I have introduced a mechanism (dependency-overrides.js) that allows the people who are motivated to do the work (the app authors who are trying to remove packages) to do all the testing for you.

The blueprint for addons already puts them into the strictest possible conditions (like disabling prototype extensions) for testing against the widest possible set of Ember apps. What I'm proposing is not different than that. When it's time to add a new ember-try scenario for the first release that offers @ember/core, best practice will be to just test against @ember/core instead of ember-source so that you would see right away if you're still compatibility, making the decision to update required-core-features as easy as possible.

I hear what you are saying about completely automatic code removal being better, but it is fundamentally not possible for an awful lot of the code, short of changing Ember's programming model to be much more verbose to make every kind of dependency static and explicit.


We have already proposed making missing dependencies a hard build error. This is important so that weird and frustrating bugs don't crop up unexpectedly. But it needs to be possible to get oneself unstuck without being blocked by third-party code.

Therefore, we propose a `config/dependency-overrides.js` file that allows you to make declarations on behalf of any addon:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ember should be striving for zero configuration. This adds lots more configuration that will need to be regularly updated if a user wants to use a recent version of Ember. Right now, on Ember Observer, I did a quick search and found just 17 addons whose package.json is up to date with the most recent version of Ember.

I strongly encourage you to rethink this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of this configuration is required in any app. This is purely about making it not impossible to spend more effort if you want to make your app smaller.

Apps are free to ignore 100% of this RFC and will always be free to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

The question for me is to what extent this RFC will impact addon developers, not app developers.

@workmanw
Copy link
Contributor

This seems cool and I could see how it could make the life easier for the maintainers of Ember. That said, it seems like it would be a lot of effort and I'm struggling to see the real value it offers to Ember's end users.

@ef4
Copy link
Contributor Author

ef4 commented Dec 14, 2017

Given the given example from the RFC, let's assume Ember 3.2.0 is released and it splits out @ember/partial and @ember/prototype-extensions into their own packages. That means @ember/core no longer has the same API surface area as it did before, so it is a breaking change. Does that mean that @ember/core's major version number will have to be bumped? If so, that means the @ember/core version number will quickly fall out-of-sync with the ember-source version number. If not, that means @ember/core would not follow semver.

This is a misunderstanding of semver. "Public API" it not limited to things like which functions are available. It can include whatever other conditions you want to impose. What is important is that you state clearly what is the supported way to use the software, and anyone who follows those instructions is safe.

So no, we would not bump major versions of @ember/core when splitting out packages. That is basically the whole point of this RFC. We can just start splitting packages out of ember-source right now if we're willing to only do it at breaking releases.

@ef4
Copy link
Contributor Author

ef4 commented Dec 14, 2017

Today is common and a good practice to test your addons in beta and canary versions of Ember. The Scenario: Addon author declaring dependencies doesn't clarify enough how the required-base-features will work when testing with a version beyond stable.
I have the feeling that if we don't handle that gracefully, people who want to ride the beta-wave (not to mention canary) will have to either use ember-source to be safe, or every time they update to a new beta, as the ecosystem will naturally lag behind, they have to heavily edit the config/dependency-overrides.js to satisfy all the unmet new packages.

The important thing here is that in none of those situations is anybody worse off than they are right now.

Even if you're using @ember/core and removing some packages, you can always safely upgrade to a newer version with more split-out packages regardless of what addons you're using. The upgrade will simply put those packages into your app's dependencies and everything continues working the way it did before.

The only time you need to do extra work is if you get motivated to try to remove some of those newly split-out packages from your app.

@Kerrick
Copy link

Kerrick commented Dec 14, 2017

This is a misunderstanding of semver.

If it's a misunderstanding of semver, it is likely a common one. Everything from third-party tools like David DM and GreenKeeper to the ^ used by yarn and npm assumes that it's safe to upgrade from 3.2.0 to 3.6.0 without breaking your build. That assumption would no longer apply if upgrading @ember/core from 3.2.0 to 3.6.0 would break without either using the ember-cli upgrade tool or manually adding more dependencies.

you can always safely upgrade to a newer version with more split-out packages regardless of what addons you're using.

*If you use the built-in tool and don't use any third-party tooling that has been built around the common community understanding of semver.


I'm all in for splitting into multiple packages, but I think that serious thought needs to be given to allowing the major version numbers of ember-source and @ember/core to drift. Is it really so bad that ember-source@3.6.0 depends on @ember/core@15.0.1, @ember/string@3.1.0, etc?

Google does a similar thing with material-components-web -- each component (equiv. to @ember/core or @ember/string) releases a major version number when its own API has breaking changes, and the "wrapper library" (equiv. to ember-source or the proposed @ember/all) has its own separate release cycle.

@ef4
Copy link
Contributor Author

ef4 commented Dec 14, 2017

This seems cool and I could see how it could make the life easier for the maintainers of Ember. That said, it seems like it would be a lot of effort and I'm struggling to see the real value it offers to Ember's end users.

I agree with @chancancode that many of the apps that are using Ember today shouldn't care about this at all. You're not wrong when you question the value to current Ember users.

But the goal here is to enable a whole new class of apps to become Ember users. Apps that today would not have chosen Ember because it's "too big" or "too much to learn". We can create a more comfortable adoption path for all those apps.

@Kerrick
Copy link

Kerrick commented Dec 14, 2017

But the goal here is to enable a whole new class of apps to become Ember users. Apps that today would not have chosen Ember because it's "too big" or "too much to learn". We can create a more comfortable adoption path for all those apps.

If the hope is that these sub-packages can become an adoption path for new users to the Ember ecosystem, it’s even more important that they follow the common version of semver on their own, without breaking changes inside a single major release.

Let’s say a creator of that whole new class of apps decides to use @ember/string@3.6.0 and installs it as ^3.6.0 in their non-Ember app. They don’t use ember-cli-upgrade because they don’t use Ember CLI or most of the rest of the Ember ecosystem.

Now let’s say that @ember/string@3.7.0 removes the pluralize and singularize methods to extract them into @ember/inflection instead. Now we’ve got a user who has a broken application because their build system auto-updated @ember/string from 3.6.0 to 3.7.0 which removed public API methods. That user feels burned by this, so they may never continue exploring Ember and become a user.

In an alternate world, @ember/string was bumped to 4.0.0 instead of 3.7.0 when removing those methods. The user’s app never broke, and their tooling let them know that @ember/string was out-of-date. They read the release notes and see a note about installing @ember/inflection if they want to keep the removed methods. The user can make an informed decision and feels much more confident in the Ember ecosystem than if their build had broken out from under them.

@ef4
Copy link
Contributor Author

ef4 commented Dec 14, 2017

@Kerrick ah, you are hitting on some important points I did not consider.

I should probably say explicitly that the upgrade rule applies to @ember/core itself only. We would not be splitting code out of @ember/string without bumping major versions.

Your point still stands that @ember/core having a weird upgrade policy is suboptimal from a compatibility-with-the-wider-world perspective. We just face a fundamental tradeoff here. We have a particular class of changes we want to make that can be 100% automated in a reliable way. And we will always have another class of changes that are truly breaking. We need to communicate the difference to users.

We are also working within the limitations of NPM, which makes meta-packages not really work well. It would be ideal to just have an ember package that depends on all the others, such that we can rearrange the others however we want, and users who depend on the meta package wouldn't care. The problem is that you can't actually force npm to do that properly unless you do it with peerDeps, and npm gives you no help in solving the constraint problem of getting a set of peerDeps that all approve of each other.

If somebody wants to figure out how to make a meta package work well, I would be happy with that alternative. I have gone pretty far down the path of trying to design that, and each time discovered I would need to do "weird" things that NPM doesn't normally do to make it work.

@mehulkar
Copy link
Contributor

What's the minimum package set that makes Ember, "ember"? I had thought until now that ember-source was the most minimal. What limited subset would @ember/core actually contain? (And would any of the other exploded packages actually be "ember"? E.g. @ember/string doesn't sound like ember, it sounds like a string library that happens to be published in ember's namespace, and that maybe Ember.js the framework uses.

@locks
Copy link
Contributor

locks commented Dec 14, 2017

What's the minimum package set that makes Ember, "ember"? I had thought until now that ember-source was the most minimal.

That is the question, isn't it ;)

How I view this initiative is that we want to allow users to opt into a smaller subset of Ember than ember-source. I think the examples in the RFC of @ember/partials and @ember/prototype-extensions illustrate nicely the sort of packages that a user might want to opt out ahead of their removal from ember.js, which would also be hard to do via tree shaking mechanisms as their use is fairly implicit.

(And would any of the other exploded packages actually be "ember"?

I am not sure I fully understand this point, as we have already broken Ember into separate packages.
What does "being ember" mean to you?

E.g. @ember/string doesn't sound like ember, it sounds like a string library that happens to be published in ember's namespace, and that maybe Ember.js the framework uses.

Hhhmm… ;)

@vasilakisfil
Copy link

If you can folks make it possible to extract ember-data outside ember to use it as a standalone API wrapper, I think it will be huge win. Apart from that, splitting ember into packages seems quite reasonable and the way to go.

@jnfingerle
Copy link

I do understand and agree that this RFC is in line with "vanilla" SemVer, but as @chancancode wrote:

In practice all node packages have the implicit requirement of "the only supported way to upgrade is to use an NPM-compatible package manager" (where "NPM-compatible package manger" is not even that well defined).

So, in essence, this proposal would introduce a way to use Ember, that is in fact outside the "NPM-compatible package manger"-ecosystem, but looks weirdly like it was part of it. @Kerrick has given a perfect example (Strings etc) how that would hurt. One way to deal with this would be to have @ember/core versions out of sync with ember-source etc. As I understand that this hurts in other ways, why not start with 0.x.y versions for @ember/core while there are still many packages factored out, and start stable version numbers in sync with ember-source when the main work is done, but in accordance to the classic npm notion of SemVer?

@workmanw
Copy link
Contributor

@ef4

But the goal here is to enable a whole new class of apps to become Ember users. Apps that today would not have chosen Ember because it's "too big" or "too much to learn". We can create a more comfortable adoption path for all those apps.

Honestly this feels like a facade. I can't imagine someone deciding, "I'm just going to pick up the Ember core and the router, but not use services or components". Because even if these things are decoupled at the package level, they're still part of the same ecosystem. Guides, Docs, Tutorials, Addons, etc would all have to take extreme care to explain and enforce this.

You're not wrong when you question the value to current Ember users.

It's also about the time and resources that go into this project that could be spent elsewhere. Not just the author's time, but those who review code changes, testing, documenting, reporting bugs that arise, etc.

@ef4
Copy link
Contributor Author

ef4 commented Dec 14, 2017

@workmanw

Honestly this feels like a facade. I can't imagine someone deciding, "I'm just going to pick up the Ember core and the router, but not use services or components"

The likely scenario is to start with just components. @ember/core would converge with GlimmerJS.

I absolutely agree with the premise that most real apps will eventually need most of what's in Ember. But the problem is lots of people don't understand that when they're getting started, so they pick a component-only library over Ember, and then end up reinventing all the other stuff as they go, much to their lost productivity and stability, and to our loss as a smaller community.

It's also about the time and resources that go into this project that could be spent elsewhere

This proposal is deliberately intended as infrastructure that should make everything else go faster, not slower. That is why I wrote this RFC in the first place. I don't happen to be a user that is obsessed with byte size. I'm motivated by strategic efforts to help the whole community ship faster.

There is still plenty of old and broken stuff floating around in Ember that nobody has stepped up to fix because there's no near-term payoff, since even deprecated stuff needs to stay around for quite a long time. This proposal creates a near-term payoff to fixing.

It also gives us general-purpose tools for backward compatibility, such that all future changes get easier to make, because "how to swap out the old code without breaking apps" becomes a problem with a standard solution. This lowers the amount of effort required for lots of features that people want.

It also creates a clear path for advanced users to experiment with next-generation replacements for things like the router. The more experimentation we unlock, the faster we can stabilize new features and bless them as the official release.

Your questions are good ones and they are forcing me to be more clear than I was in the original doc. The highest-level goal of this proposal is to make it easier to ship improvements in Ember, so that more people can overcome the intimidation and effort barriers required to help.

@kellyselden
Copy link
Member

I believe there were some good points brought up by @Turbo87 above that haven't been addressed regarding peer deps.

@wycats
Copy link
Member

wycats commented Feb 5, 2018

@kellyselden

At least once I got peer deps unsatisfied warnings from NPM, where I knew my setup was more correct than NPM's algorithm

So here's the thing: virtually nobody can reasonably make this part of their normal workflow. Trying to manually resolve graphs of dependencies, while allowing duplicates "when appropriate", and resolving peer dependencies manually at the top level is just a nonsense requirement that nobody can do.

The original version of this proposal tried to avoid that problem by moving the resolution for this particular problem into side-configuration, so that Ember could handle the resolution for you and avoid the brokenness of npm's peer dep warnings.

However, there was strong pushback that we should just "go with npm" and not "reinvent the wheel". I'll accept either of the two, but if we're gonna go with peer dependencies, we need to be able to assume they work. If we think people will need to manually do work to get around brokenness, we really need to use the side-config approach.

So which is it?

@jnfingerle
Copy link

@wycats

I'll accept either of the two, but if we're gonna go with peer dependencies, we need to be able to assume they work.

If npm really is that broken it has to be fixed. If that isn't possible, by all means, run your own but don't do it inside the existing npm ecosystem disguised as part of it when it isn't.

Maybe the peerDependencies approach with an "I know what I'm doing and don't want hard errors" config option might be a solution?

@Turbo87
Copy link
Member

Turbo87 commented Feb 16, 2018

Maybe the peerDependencies approach with an "I know what I'm doing and don't want hard errors" config option might be a solution?

That's essentially what I meant with "escape valve" in #284 (comment)

One example where the npm peer deps warning was entirely wrong for me:

  • eslint-plugin-qunit (with "eslint": ">=3.18.0 <5.0.0" peer dep)
  • ember-cli-eslint depends on broccoli-lint-eslint depends on eslint@4

The nested eslint version was correct and satisfying the eslint-plugin-qunit peer dep constraint, but npm was still complaining because I don't have eslint as a direct dependency.

These things seem to happen quite often to me and I'm worried that enforcing peer deps in the same way will create much more pain than it solves. I'm absolutely okay with displaying warnings for them, but I'm opposed to hard errors if there is no way to turn them off.

Sidenote: we should consider how to implement this in Ember CLI without losing startup performance

@tomdale
Copy link
Member

tomdale commented Feb 19, 2018

@Turbo87 In your eslint example, isn't the issue resolved by including eslint as a direct dependency of the app? To me, this seems okay because there are no guarantees that a future release of broccoli-lint-eslint won't change its version to something that no longer satisfy's eslint-plugin-qunit's peer dependency.

In other words, it seems okay to me to say that transitive dependencies are not allowed to fulfill peer dependency requirements. The worst case scenario is you can just adopt the dependency with the same version range as the other dependency bringing it in transitively. In your example, you could add eslint@4 to your package.json and dependencies would resolve the same as before, but now satisfy the peer dependency.

Is there a downside to this that I'm missing?

@Turbo87
Copy link
Member

Turbo87 commented Feb 19, 2018

isn't the issue resolved by including eslint as a direct dependency of the app?

then something like yarn run eslint would potentially run a different ESLint version than the one that broccoli-lint-eslint is resolving to, which would not be such a great solution unfortunately.

this seems okay because there are no guarantees that a future release of broccoli-lint-eslint won't change its version

I think the expectation is that if broccoli-lint-eslint (and ember-cli-eslint) update to a new major eslint version they would also bump majors. They are essentially exposing the ESLint APIs directly to the users so any breaking API change (including rule changes) from eslint would also be a breaking change for those libs.

In your example, you could add eslint@4 to your package.json and dependencies would resolve the same as before, but now satisfy the peer dependency.

Until someone updates eslint to 5.0.0 and forgets about ember-cli-eslint. Essentially it's just a workaround for the "broken" warnings in npm and yarn, but at least they're just warnings, not hard errors.

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2018

Removing Final Comment Period so that we can address recent feedback...

@NullVoxPopuli
Copy link
Contributor

Has the feedback been addressed?

@ef4
Copy link
Contributor Author

ef4 commented Mar 20, 2020

Update: this RFC has been on the back burner for a while but it is still quite relevant and the text holds up well. I still think it's a good idea, I just haven't been actively championing it as working on embroider comes higher in my priorities list.

The only major remaining objection was concern about making invalid peerDependencies into errors. I accept that the NPM ecosystem is riddled with problems that make keeping peerDeps valid difficult in general. My suggestion is:

  • reduce the scope of that feature so that it only errors on unsatisfied peerDeps in Ember's own packages.
  • provide an environment variable to suppress the error so that people who are doing advanced debugging of the ember packages themselves can test scenarios that involve mismatched package versions.

If anybody has some time to take over and incorporate that into the text, I think this RFC is still right near the home stretch.

Update RFC section on inter-package dependencies
@hakilebara
Copy link

If the latest feedback has been addressed by #646, it is OK to move this RFC along its path to be merged?

"ember-addon": {
"required-core-features": "0.4.0",
"required-additional-features": ["@ember/routing", "@ember/prototype-etxensions", "@ember/string"]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just flagging that there's going to be some amount of overlap with deprecation staging config here #649. We discussed making it possible to identify "parts of ember they do not use", but decided against it. Nothing really to do at the moment, since it's not really feasible to try to expand the scope of both RFCs and there isn't anything inherently conflicting yet. Just wanted to let you know. cc @pzuraq @rwjblue.


We have already proposed making missing dependencies a hard build error. This is important so that weird and frustrating bugs don't crop up unexpectedly. But it needs to be possible to get oneself unstuck without being blocked by third-party code.

Therefore, we propose a `config/dependency-overrides.js` file that allows you to make declarations on behalf of any addon:
Copy link
Contributor

Choose a reason for hiding this comment

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

This was also a topic of discussion in #649. @wycats was of the opinion that overriding an addon with config is not the right path to promote. If an addon is out of date, the incentives should be to go fix the addon and use a soft fork in the meantime.


# How We Teach This

The most important message we need to teach app developers is: if you want to use `@ember/core` during the initial phase, you should use `ember-cli-update` whenever you're changing your Ember version. As long as apps follow that advice, they should experience the same level of stability they would get from `ember-source`. As people begin to experiment with removing Ember packages, our best teaching opportunities are very clear and helpful feedback from `ember-cli` whenever they have a dependency issue, as illustrated in some of the example messages in this document.
Copy link
Contributor

Choose a reason for hiding this comment

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

ember-cli-update should maybe become ember update. Opened #653.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch.

@NullVoxPopuli
Copy link
Contributor

What's left on this? 🤔

@jelhan
Copy link
Contributor

jelhan commented Jan 29, 2022

What's left on this? 🤔

Has implementation even started? As far as I know ember-source is still the only way to depend on Ember.

Some preparation work has been started for sure. Some features earlier included in ember-source package have been replaced by standalone packages. @glimmer/component, @glimmer/tracking and @ember/test-helpers are some examples.

But a common Ember all still relies heavily on fake packages like @ember/service, @ember/routing and @ember/controller. All of them are still provided by ember-source.

It even gets worse if also pulling all the fake packages into account, which are less commonly used by an application itself. By far the most packages listed in Ember API docs are fake packages provided by ember-source. It's still a long, long way until ember explode is possible as described in this RFC.

But to be honest I'm not sure if that's still the right target. Maybe instead of splitting up existing Ember we could continue to implement new replacement as standalone packages. It's a slow transition path.

But I'm also not sure if there are that many use cases which require a developer to start with @glimmer/component and install all their way up to Ember. If it is about bundle size, maybe we could achieve the same quicker focusing on strategies like dead-code elimination (tree-shaking), deprecating features in core like @ember/component and excluding them from the built based on optional features.

I don't have much insights in core development. But it seemed as if doing so worked out quite well in the last two years.

@ef4
Copy link
Contributor Author

ef4 commented Jan 30, 2022

I still think the high-level goals of the RFC are good. I think we would want to do an updated iteration that takes advantage of all the progress we've already made recently across the ecosystem.

For example, we're starting to see addons shipping in v2 format now. Since V2 addons have to ship their code in a form that's statically analyzable, we can reliably tell which Ember packages they really use. So they wouldn't need the required-core-features and required-additional-features stuff that's in this RFC.

As of now, that would cover addon usage of Ember modules via JS imports, but when you combine it with https://github.com/emberjs/rfcs/blob/master/text/0496-handlebars-strict-mode.md via something like #779, we would have full visibility into what parts of Ember are used by any addon. That solves the hard migration problem that this RFC deals with.

Also, as of Ember 4.0 we made sure that apps are required to have support for v2 addon format via ember-auto-import >=2. That means we're free to ship ember-source itself as a v2 addon, which makes all usage of ember modules pay-as-you-go.

So I think we're actually making huge progress in moving the ecosystem toward making it possible to ship ember as "real" packages without disruption.

The next ecosystem-wide thing I'd like to do that would protect users from breakage as you move toward ember being "real packages" made out of "real modules" is deprecating all usage of require and define. But we need to ship the replacement first, which probably means making our DI system asynchronous and enabling top-level await.

@webark
Copy link

webark commented Jan 31, 2022

If we are considering a revised RFC around this concept, would we ever consider allowing for the new "isolated" modules to not require glimmer as it's rendering engine? Or will the packages always require the assumption that the rendering engine/library is glimmer.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@ef4 should we leave this open still?

@wagenet wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022
@ef4
Copy link
Contributor Author

ef4 commented Dec 7, 2022

This is still a good goal, but the details have likely changed since this was written and an updated proposal would be needed to move this forward.

The crux of the issue is that to maintain correctness while using separate packages we really need reliable peer dependencies, and NPM clients are still pretty terrible at peer dependencies, though the exact ways in which they are good and bad are shifting. pnpm is currently the best at correctness, but if we want to lean heavily on that we'll need an RFC asking the community to all adopt pnpm, and that might be a tough sell for some companies.

@ef4 ef4 closed this Dec 7, 2022
@dfreeman dfreeman deleted the explode branch December 7, 2022 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Proposed In the Proposed Stage T-infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.