Standarize targets by cibernox · Pull Request #95 · ember-cli/rfcs

cibernox · 2017-01-10T09:09:42Z

…ripping Ember CLI: Production code stripping

cibernox · 2017-01-10T09:14:47Z

This RFC is somewhat an extraction of RFC in PR #89, but less controversial in the sense that focuses on standarizing app targets and its syntax but leaves open how ember, ember-cli or addons should use them.

I open this in hope that it can be approved faster and independently of the advances in the babel6 migration.

Fix typo

twokul · 2017-01-11T14:58:40Z

active/0000-standarise-targets.md

+### Motivation
+
+Javascript and the platforms it runs on are moving targets. NodeJS and browsers release new
+versions every few weeks. Browsers auto update and each update bings new language features


bings -> brings?

Serabe · 2017-01-27T10:27:39Z

active/0000-standarise-targets.md

+
+The syntax for for this configuration option is even simpler, since apps will only run in a single
+version of node and developers are in control of it, so the only thing users must specify is the
+minimum supported version of node.


If this is added, I would either let the user specify the node version as they do in the package.json or specify in the name that is the minimum version acceptable. The first option is harder to implement, but people would be more used to it.

I'd suggest that the default would just do node: require('../package').engine (deferring to the engine listed in package.json).

I'm also ok with that. We can reuse it.

Serabe · 2017-01-27T10:29:38Z

active/0000-standarise-targets.md

+
+Let every addon that wants to deal with targets to have a `targets`-like option in its configuration
+instead of standardizing a single configuration option, which effectively leaves things as they are
+right now.


This option opens the door of a misconfiguration, in the sense that you might add support for X browser in autoprefixer but not in babel and suddenly, your life is much more interesting. :)

I didn't say it was a good alternative 😉 But this is the state of things right now: each addon uses it's own configuration, and possibly its own syntax.

stefanpenner · 2017-01-27T15:49:46Z

big fan of this direction!

I'll try and fine time over the next few days to thoroughly review.

cibernox · 2017-02-01T16:56:14Z

@stefanpenner any opinion on core people on this? I could work in the implementation myself. I'm craving for some tooling.

rwjblue

but this RFC doesn't impose any mandatory behavior on addons

I agree that we shouldn't (or can't) mandate anything, but we should recommend that they honor them...

rwjblue · 2017-02-02T20:32:50Z

active/0000-standarise-targets.md

+
+Developers need an easy way to express intention that abstracts them from the ever-changing
+landscape of versions and feature matrices, so this RFC proposes to introduce a universal
+configuration option to let developers express they intended targets


s/they/their/

rwjblue · 2017-02-02T20:35:05Z

active/0000-standarise-targets.md

+
+Addons that would benefit from this conventions are `babel-preset-env`, `autoprefixer`,
+`stylelint` and `eslint` (vía `eslint-plugin-compat`), but not only those. Even Ember itself could,
+once converted into an addon, take advantage of that to avoid polyfilling or even taking


it is an addon as of ember-cli@2.11 / ember-source@2.11.0 🎉

rwjblue · 2017-02-02T20:35:30Z

active/0000-standarise-targets.md

+once converted into an addon, take advantage of that to avoid polyfilling or even taking
+advantage of some DOM API (`node.classList`?) deep in Glimmer's internals.
+
+### Detailed implementation


Not sure if this was intentionally left blank?

It wasn't blank, it's just that it was an h3 and it should be an h1, and the next points are subsections.

aha! thank you!

rwjblue · 2017-02-02T20:37:48Z

active/0000-standarise-targets.md

+```
+
+This configuration must be made available to addons. It's up to the addons to use it.
+I suggest to make it available in `app.targets`


I would prefer one of these instead:

add this.targets property

this.project.targets() / this.project.targets

rwjblue · 2017-02-02T20:40:58Z

active/0000-standarise-targets.md

+
+```js
+var app = new EmberApp(defaults, {
+  targets: {


We should avoid tying this target system to the build time only ember-cli-build.js / EmberApp instance since it will be needed from places outside of the build environment. For example, when editing a project I enjoy utilizing my editors built in eslint system to indicate any linting errors/warnings that I have in my code. If I wanted to utilize eslint-plugin-compat, I would have to duplicate the configuration in ember-cli-build.js and .eslintrc.js (or change ember-cli-build.js to have multiple exports).

I'd prefer that we either:

put the config in .ember-cli file which is already in the project

put the config in another file in config/ (e.g. config/supported-targets.js)

Actually, one interesting approach of this is the one eslint-plugin-compat uses. It reads the browserlist property in the package.json.

If we are cool with reading the engines property of the package.json for the node support it might also be a reasonable place to the list of supporter browsers.

rwjblue · 2017-02-02T20:41:45Z

active/0000-standarise-targets.md

+  included(app) {
+    this._super.included.apply(this, arguments);
+
+    console.log(app.targets.browsers); // ['>2%', 'last 3 iOS versions', 'not ie <= 8']


The meaning of app changes when using things like fastboot and ember-engines, this is why I would like to decouple storing of target from app...

rwjblue · 2017-02-02T20:42:53Z

active/0000-standarise-targets.md

+
+The syntax for for this configuration option is even simpler, since apps will only run in a single
+version of node and developers are in control of it, so the only thing users must specify is the
+minimum supported version of node.


I'd suggest that the default would just do node: require('../package').engine (deferring to the engine listed in package.json).

rwjblue · 2017-02-02T20:43:37Z

active/0000-standarise-targets.md

+traditionally enforced only with peer reviews.
+
+To ease the transition Ember CLI can also, in the absence of a specific value provided by the user,
+default to a predefined matrix of browsers that matches the browsers officially supported by the framework.


👍 - But we should (in this RFC) list out the "default" configuration...

I'm not sure what Ember supports. I know that IE support starts in 9, but no idea about the policy for other evergreen browsers. Is there one? Who knows it?

stefanpenner · 2017-02-02T21:26:47Z

active/0000-standarise-targets.md

+
+```js
+var app = new EmberApp(defaults, {
+  targets: {


targets object, to be consider together? e.g. lowest common denominator between all properties, or is this 2 separate builds... I believe they should be lowest common denominator

This is affectively how things like babel-preset-env and eslint-plugin-compat works. Browserlist has an evergreen database from caniuse.com with features by browser version, so it calculates the support on a per-feature basis.

I'll add it.

Added support matrix from the browsers we test in saucelabs & travis. No mention to ios tho, but likely IE9 will be the weakest ring of the chain in everything anyway.

rwjblue · 2017-02-02T21:52:30Z

Discussed during ember-cli core team meeting today, we were generally very 👍. Main issues are:

naming ("target" is hard to use, perhaps we can do somethign like target-platforms, supported-platforms, etc...)
determine exactly where to put this config (IMO it should not be in ember-cli-build.js, but that is TBD obviously)
confirm if multiple targets are supposed to be considered as a combination (e.g. "lowest common denominator")
list the explicit default config that matches ember-source@2.11's supported platforms

kellyselden · 2017-02-03T00:03:39Z

If the default is dependent on which ember version you may be using, should it be bundled with ember (like its blueprints)? I'm not sure what this would look like in a way you could override it in ember-cli land...

cibernox · 2017-02-03T00:10:21Z

@kellyselden It could be part of ember-cli, because a ember drops browser support in major versions only, which would carry (I think) a major version bump in ember-cli as well.

kellyselden · 2017-02-03T00:14:48Z

@cibernox True, but we still allow ember 1.13 in ember-cli 2.x, and will be the same for the next cycles. Might be unnecessary friction to have more code in cli that ties to specific ember versions.

cibernox · 2017-02-03T01:01:39Z

@kellyselden So options:

Make it a part of ember-source, like blueprints. In the absence of configuration, we use those values.
We trust the file (e.g config/targets.js). This file is part of the default blueprint of an app, and lives wherever blueprints live now (I think they've extracted). If the file is not present, we don't have any default value.

I lean towards the first. At the end ember.js already has (kind of) this information about browser support in the config files for travis and saucelabs.

ro0gr · 2017-02-03T06:50:39Z

active/0000-standarise-targets.md

+This library is very powerful and popular, making relatively easy to integrate with a good amount of
+tools that use it in little time.
+
+This configuration must be made available to addons but we it's up to the addon authors to take advantage


odd "but we"

cibernox · 2017-02-03T09:40:34Z

Feedback applied:

Per slack bikeshedding, the configuration will be in this.project.targets. The object in that property will have browsers and node properties. There's another RFC Multiple Build Targets #93 that wants to introduce another kind of targets. The chamption (@kratiahuja) seems happy to use this.project.targets.platforms for its concept.
Added data of where the config must be placed. Preferred option: config/targets.js, similar to config/ember-try.js, which is also used to build a support matrix, but of ember versions instead of browser versions.
Confirmed Minimum Common Denominator of the approach, and added an example.
Added list default supported browsers, extracted from the browsers tested in ember in travis and saucelabs.

kanongil · 2017-02-03T15:41:06Z

Does the node property encompass supporting alternative engines, like node-chakracore?

cibernox · 2017-02-03T16:09:47Z

@kanongil The node property is basically require('package.json').engines. Do you know if node-chakracore is specified that way?

rwjblue · 2017-02-09T22:04:16Z

Discussed in ember-cli core meeting today, we are 👍 on advancing this to final comment period.

One thing that we would like to see before landing the RFC (after FCP is finished and whatnot) is a spike of the implementation in a PR to ember-cli. This is just to confirm that all of the constraints listed here are addressed and we don't find flaws.

I believe that I will have time to spike this out in the next couple of days...

cibernox · 2017-02-09T22:43:09Z

Nice! If you don't let me know and I can try.

Cryrivers · 2017-02-15T14:00:50Z

speaking of targets, is there any consideration for targets like Cordova, Electron, or "Ember Native" (maybe in the future)?

rwjblue · 2017-02-15T14:30:25Z

@Cryrivers - Yes, but that is a somewhat separate RFC (see #93 for some background).

cibernox · 2017-02-15T14:46:20Z

@Cryrivers @rwjblue Actually after some investigation there is a small overlapping on the concepts. Some libraries like babel-preset-env consider electron a target, just like any other browser, and use https://github.com/kilian/electron-to-chromium to translate from the version of electron to the equivalent chromium version to know if a feature is supported.

The targets this RFC targets are the javascript runtimes in which this app is going to run. That is the browser or electron version, or even node version in fastboot.

A separate concept is "platforms" (name to be bikeshedded) that only makes sense when you are going to wrap your app and you are going to write javascript code that bridges to the OS using electron/cordoba APIs.

In summary, in this RFC a target is "where my Ember app code is going to run on". The electron specific code is probably going to live in a separate folder and RFC #93 wants to solve that. It's an area I don't have any expertise on, TBH.

kanongil · 2017-02-15T15:29:28Z

I'm a bit concerned that the project.targets.browsers naming is a bit too generic. I would prefer that it signals that it only applies to the javascript runtime. Maybe just prefix is with "js" so that it becomes project.jsTargets.browsers ?

As it is, you can confuse it with other runtime capabilities, like css capabilities, which e.g. https://github.com/kimroen/ember-cli-autoprefixer handles.

rwjblue · 2017-02-15T15:35:46Z

@kanongil - I believe that is entirely the point. We want it to be used for both JS transpilation and CSS transpilation targeting. This is mentioned in the Motivation and Detailed Design sections of this RFC.

cibernox · 2017-02-15T15:44:37Z

@kanongil exactly. It's not scoped to js. CSS is part of the plan. My example if my app drops support for IE11 then my ember-css-linter (invented addon) can warn me wherever I use -ms-user-select: none, since I don't need the prefix anymore.

cibernox · 2017-02-16T09:46:49Z

I have a working implementation of this ember-cli/ember-cli#6776
It was pretty simple to implement.

I've also spiked how ember-maybe-import-regenerator (used by ember-concurrency) could use it to conditionally avoid including the regenerator polyfill if the built matrix supports generator functions.

rwjblue · 2017-02-17T16:44:42Z

Discussed in ember-cli core team meeting yesterday, and with some small (scope narrowing) changes we are ready to land this.

Specifically, we would like to restrict targetsto only include browsers at this point (IOW, remove node section). A future RFC can add support for things like fastboot / cordova / etc, and removing node here shouldn't affect the primary motivators of the RFC.

@cibernox - Let me know when you have had a chance to update that, and we are ready to land this...

cibernox · 2017-02-17T23:20:49Z

@rwjblue Amended the RFC to leave node/fastboot out of this RFC, so will cross that bridge another day.

Implement targets RFC This PR implements ember-cli/rfcs#95 (still unmerged). API example ```js app.project.targets; // { browsers: [...], node: '6.6.0' }; ```

kanongil · 2017-04-03T13:36:10Z

In case you missed it, I posted some comments on javascript issues in #104.

Standarize targets

nathanhammond and others added 3 commits December 1, 2016 23:25

Merge pull request ember-cli#50 from GavinJoyce/gj/production-code-st…

e896286

…ripping Ember CLI: Production code stripping

Merge branch 'master' of github.com:ember-cli/rfcs

dbf65d0

Standarize targets in configuration

3f5e630

cibernox and others added 3 commits January 10, 2017 14:09

Add entry about how to support node

1a36ee6

Fix typo

73eb8f3

Merge pull request #1 from mariogintili/patch-1

d6bbc38

Fix typo

twokul reviewed Jan 11, 2017

View reviewed changes

Fix typos

5a3dbab

Serabe reviewed Jan 27, 2017

View reviewed changes

Improve some sentences

05a4cf5

rwjblue reviewed Feb 2, 2017

View reviewed changes

stefanpenner reviewed Feb 2, 2017

View reviewed changes

cibernox added 3 commits February 2, 2017 23:02

Some improvements

e2b5779

Bikesheding how/where this configuration lives

de5447f

Better wording

6218483

Mention official support matrix

239e717

Clarify minimum common denominator

77f28a9

ro0gr reviewed Feb 3, 2017

View reviewed changes

Improve RFC in general

facacb6

rwjblue added the Final Comment Period label Feb 9, 2017

cibernox mentioned this pull request Feb 15, 2017

Implement targets RFC ember-cli/ember-cli#6776

Merged

Remove node/fastboot from the scrope of this RFC

21b6102

rwjblue merged commit d9e2e5f into ember-cli:master Feb 18, 2017

cibernox deleted the standarize-targets branch February 24, 2017 13:04

kanongil mentioned this pull request Mar 28, 2017

Problems with standardized targets RFC #95 and javascript #104

Closed

kategengler pushed a commit to kategengler/rfcs that referenced this pull request Jan 19, 2019

Merge pull request ember-cli#95 from cibernox/standarize-targets

9b94a35

Standarize targets

Uh oh!

Conversation

cibernox commented Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cibernox commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner commented Jan 27, 2017

Uh oh!

cibernox commented Feb 1, 2017

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Feb 2, 2017

Uh oh!

kellyselden commented Feb 3, 2017

Uh oh!

cibernox commented Feb 3, 2017

Uh oh!

kellyselden commented Feb 3, 2017

Uh oh!

cibernox commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox commented Feb 3, 2017

Uh oh!

kanongil commented Feb 3, 2017

Uh oh!

cibernox commented Feb 3, 2017

Uh oh!

rwjblue commented Feb 9, 2017

Uh oh!

cibernox commented Feb 9, 2017

Uh oh!

cibernox commented Jan 10, 2017 •

edited

Loading

cibernox commented Feb 15, 2017 •

edited

Loading