[CLEANUP] drop IE8 stuff #3345
[CLEANUP] drop IE8 stuff #3345
Conversation
stefanpenner
commented
Jun 15, 2015
stefanpenner
commented
- first pass
- run against ember canary 4 realz
| import Adapter from "ember-data/system/adapter"; | ||
|
|
||
| /** | ||
| `DS.FixtureAdapter` is an adapter that loads records from memory. |
stefanpenner
force-pushed
the
drop-ie8
branch
4 times, most recently
from
a7484bb
to
735ae3f
Compare
stefanpenner
force-pushed
the
drop-ie8
branch
2 times, most recently
from
9114391
to
ecb0c11
Compare
|
Please don't merge until we ship a 2.0 beta. 1.13 needs to support IE8. |
|
Having 1.13 supporting IE8 would be great thanks :-) |
|
@MichaelVdheeren Ya this is for the version that will work with ember 2 |
|
When is 1.13 branched? |
| forEach(payload, function(single) { | ||
| single.type = self.modelNameFromPayloadKey(single.type); | ||
| }); | ||
| payload.forEach((single) => single.type = this .modelNameFromPayloadKey(single.type)); |
There was a problem hiding this comment.
is this whitespace intentional?
There was a problem hiding this comment.
No, it's a typo. I'm surprised jshint didn't warn about that.
|
Can't connect wifi currently. I'll update tonight |
| @@ -27,7 +27,7 @@ var get = Ember.get; | |||
| @class BuildURLMixin | |||
| @namespace DS | |||
| */ | |||
| var BuildURLMixin = Ember.Mixin.create({ | |||
| export default Ember.Mixin.create({ | |||
There was a problem hiding this comment.
really dislike this style as it makes the code less greppable and you also have to undo it later if you have to call reopen or reopenClass.
There was a problem hiding this comment.
<ctrl-p> buildurlm (or w/e fuzzy file search the user has) nicely targets the file that it comes from, not confused with consumers.
There was a problem hiding this comment.
That only works when the file is named appropriately (which seems fine in this specific case), often we have ember-metal/utils which is useless for fuzzy file finders.
There was a problem hiding this comment.
@rwjblue you may have identified a good refactoring, although unrelated to this point.
default export should really have some extremely strong relation to the file it lives in. As that is it's public addressable location.
for example:
// my/adapter.js
import adapter from '../adapter/base'
export default class extend Adapter {
}The above code relies on strong assumptions, these strong assumption actually help discoverability.
Given that:
- default exports must have a strong relation to the relative on-disk location
- consumption is also based on the relative on-disk location ` import foo from 'relative/on-disk/location'
- source maps indicate relative on-disk location
It appears that the primary identifier is actually this relative on-disk location, any additional hints are redundant and potentially anti-pattern encouraging as consumers relying on these secondary hints are not benefiting from the consistently addressable patterns that aims to stand true regardless of the use-case.
There was a problem hiding this comment.
if cases exist where the path is not strongly related, that should be considered a bug. If other cases exist where the path isn't searchable, that also appears like a bug.
Even without source maps, searching the built-output for the on-disk location will still reveal the AMD module, as we opt to fully named and resolved modules.
In addition, searching this way can also reveal importers.
There was a problem hiding this comment.
Looks like this line is the only chase where @stefanpenner changed a file to use export default without a name. I think we should merge this pr as is to unblock users who want to test Ember 2.0.
We can bike shed this topic as a separate issue since the Ember Data codebase already contains a mix of both styles.
There was a problem hiding this comment.
I am unsure why this is controversal, all ember-cli default blueprints do this, and the majority of ember itself does this.
Lets just merge and move on.
There was a problem hiding this comment.
Who can merge it/what's stopping it from being merged? Let's get this in there
|
Keen on this.. ember data canary doesn't work with ember canary |
Ya, this PR fixes that. |
stefanpenner
force-pushed
the
drop-ie8
branch
2 times, most recently
from
ec9ed44
to
5f35670
Compare
|
rebase + green on both ember 1.13 (without IE8) and canary |
|
I believe people are anxious to try out ember#canary, this is likely one of the blockers. as we have branched to v1.13, this should be safe for master. If there are other blockers please let me know. |
| data = Ember.A(relationshipHash).map(function(item) { | ||
| return this.extractRelationship(relationshipMeta.type, item); | ||
| }, this); | ||
| data = relationshipHash.map((item) => this.extractRelationship(relationshipMeta.type, item)); |
There was a problem hiding this comment.
Ember.A() was removed during the cleaning. Was it intentional or by mistake?
Indeed, now it breaks when you normalize a record with hasMany relationships which are not defined in the payload.
REST payload example:
{
'post': [{
'id': 2,
'title': 'Post #2',
'comments': [] // OK in both 1.13 and 2.0
}, {
'id': 3,
'title': 'Post #3',
'comments': null // OK in 1.13 *but* KO in 2.0
}, {
'id': 4,
'title': 'Post #4'
// no comments = OK in both 1.13 and 2.0
}]
}Full tests on JSBin:
- ED 1.13: http://emberjs.jsbin.com/puguhi/edit?html,js,console,output
- ED 2.0: http://emberjs.jsbin.com/towofa/edit?html,js,console,output
For now, the workaround to ensure compatibility with existing data is to override the default normalization and force the undefined relationships to be empty arrays, or even completely remove them.
