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

Implement targets RFC #6776

Merged
merged 1 commit into from Feb 21, 2017

Conversation

Projects
None yet
4 participants
@cibernox
Contributor

cibernox commented Feb 15, 2017

This PR implements ember-cli/rfcs#95 (still unmerged).

API example

app.project.targets; // { browsers: [...], node: '6.6.0' };
Show outdated Hide outdated lib/models/project.js
let appTargets;
if (existsSync(`${targetsPath}.js`)) {
appTargets = JSON.parse(fs.readFileSync(`${targetsPath}.js`, 'utf8'));

This comment has been minimized.

@rwjblue

rwjblue Feb 15, 2017

Contributor

I believe this should use require since the targets file isn't valid JSON and we want to allow usage of node-land things anyways.

@rwjblue

rwjblue Feb 15, 2017

Contributor

I believe this should use require since the targets file isn't valid JSON and we want to allow usage of node-land things anyways.

This comment has been minimized.

@cibernox

cibernox Feb 15, 2017

Contributor

Done, but test fail. I've created the file with the right content, but I can't require it.
I suspect that must be something obvious related with how the fake filesystem works in tests.

@cibernox

cibernox Feb 15, 2017

Contributor

Done, but test fail. I've created the file with the right content, but I can't require it.
I suspect that must be something obvious related with how the fake filesystem works in tests.

Show outdated Hide outdated blueprints/app/files/config/targets.js
/* eslint-env node */
module.exports = {
browsers: [] // ["last 2 versions", "safari >= 7"]

This comment has been minimized.

@rwjblue

rwjblue Feb 15, 2017

Contributor

Let's inline the default here also (instead of the comment). This lets us update the default for new apps without potentially breaking existing ones.

@rwjblue

rwjblue Feb 15, 2017

Contributor

Let's inline the default here also (instead of the comment). This lets us update the default for new apps without potentially breaking existing ones.

Show outdated Hide outdated lib/models/project.js
@return {Object} Targets object
*/
get targets() {
let configPath = 'config';

This comment has been minimized.

@rwjblue

rwjblue Feb 15, 2017

Contributor

we should memoize this to avoid reading from disk on every access

@rwjblue

rwjblue Feb 15, 2017

Contributor

we should memoize this to avoid reading from disk on every access

This comment has been minimized.

@cibernox

cibernox Feb 15, 2017

Contributor

Done.

@cibernox

cibernox Feb 15, 2017

Contributor

Done.

@cibernox

This comment has been minimized.

Show comment
Hide comment
@cibernox

cibernox Feb 15, 2017

Contributor

All feedback applied. There is still the issue of the failing test because this.require can't find the ${projectPath}/config/targets, while fs.readFileSync(${projectPath}/config/targets.js) can read it.

Contributor

cibernox commented Feb 15, 2017

All feedback applied. There is still the issue of the failing test because this.require can't find the ${projectPath}/config/targets, while fs.readFileSync(${projectPath}/config/targets.js) can read it.

@cibernox

This comment has been minimized.

Show comment
Hide comment
@cibernox

cibernox Feb 16, 2017

Contributor

I've fixed the tests and validated its behaviour in app (installing it from my github).

Open implementation questions:

  • At the moment the config/targets.js exports a simple POJO. /config/ember-try.js follows the same approach. config/environment.js instead exports a function that when called with the environment returns the POJO. If you think that function-that-returns-pojo provides some value now it's the time.

  • When the package.json doesn't have an engines property the targets don't include the node property. I can leave it that way or I can return the minimum supported version of node, ATM node 4, although this supporter version afaik refers to ember-cli, and ember itself is not tested against any version of node so I lean towards just not including it.

  • The POJO I return is currently memoized and it's not frozen. In its current implementation an addon could grab it and modify it. I'm sitting on the fence wether of not that is a weakness or a strenght. By example, I can imagine ember-fastboot being node 6+ despite of ember-cli being node4+. Also I can imagine ember-electron doing something similar. I'd rather leave it unfrozen for now.

Contributor

cibernox commented Feb 16, 2017

I've fixed the tests and validated its behaviour in app (installing it from my github).

Open implementation questions:

  • At the moment the config/targets.js exports a simple POJO. /config/ember-try.js follows the same approach. config/environment.js instead exports a function that when called with the environment returns the POJO. If you think that function-that-returns-pojo provides some value now it's the time.

  • When the package.json doesn't have an engines property the targets don't include the node property. I can leave it that way or I can return the minimum supported version of node, ATM node 4, although this supporter version afaik refers to ember-cli, and ember itself is not tested against any version of node so I lean towards just not including it.

  • The POJO I return is currently memoized and it's not frozen. In its current implementation an addon could grab it and modify it. I'm sitting on the fence wether of not that is a weakness or a strenght. By example, I can imagine ember-fastboot being node 6+ despite of ember-cli being node4+. Also I can imagine ember-electron doing something similar. I'd rather leave it unfrozen for now.

@cibernox cibernox referenced this pull request Feb 16, 2017

Merged

Standarize targets #95

@cibernox

This comment has been minimized.

Show comment
Hide comment
@cibernox

cibernox Feb 17, 2017

Contributor

@rwjblue Removed references to node since it was withdrawn from the RFC.

Contributor

cibernox commented Feb 17, 2017

@rwjblue Removed references to node since it was withdrawn from the RFC.

@cibernox

This comment has been minimized.

Show comment
Hide comment
@cibernox

cibernox Feb 21, 2017

Contributor

@rwjblue Added to experiments.

Contributor

cibernox commented Feb 21, 2017

@rwjblue Added to experiments.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Feb 21, 2017

Contributor

@homu r+

Contributor

rwjblue commented Feb 21, 2017

@homu r+

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Feb 21, 2017

Contributor

📌 Commit 25e24d9 has been approved by rwjblue

Contributor

homu commented Feb 21, 2017

📌 Commit 25e24d9 has been approved by rwjblue

homu added a commit that referenced this pull request Feb 21, 2017

Auto merge of #6776 - cibernox:implement-targets-rfc, r=rwjblue
Implement targets RFC

This PR implements ember-cli/rfcs#95 (still unmerged).

API example

```js
app.project.targets; // { browsers: [...], node: '6.6.0' };
```
@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Feb 21, 2017

Contributor

⌛️ Testing commit 25e24d9 with merge 2db75f1...

Contributor

homu commented Feb 21, 2017

⌛️ Testing commit 25e24d9 with merge 2db75f1...

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Feb 21, 2017

Contributor

☀️ Test successful - status

Contributor

homu commented Feb 21, 2017

☀️ Test successful - status

@homu homu merged commit 25e24d9 into ember-cli:master Feb 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Turbo87 Turbo87 added the Enhancement label Feb 21, 2017

@cibernox cibernox deleted the cibernox:implement-targets-rfc branch Mar 13, 2017

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