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

import require #4135

Merged
merged 1 commit into from
Feb 12, 2016
Merged

import require #4135

merged 1 commit into from
Feb 12, 2016

Conversation

jmurphyau
Copy link
Contributor

The global require in this file causes an error when using babel 6 with the transform-es2015-modules-amd plugin

fixes #4134

@stefanpenner
Copy link
Member

i believe you should be able to actually import require

import require from 'require'

does that still cause an issue?

@jmurphyau
Copy link
Contributor Author

I tried that originally and travis tests failed so switched to the way it is now

@fivetanley
Copy link
Member

Is there a reason we can't just import Model from 'ember-data/model'; instead?

@bmac
Copy link
Member

bmac commented Feb 9, 2016

Originally when this code was added import Model from 'ember-data/model'; resulted in a circular reference which was breaking the tests.

@pangratz
Copy link
Member

pangratz commented Feb 9, 2016

Just an idea since the Model is only needed for the assertion, what about a duck typing approach of checking if record.hasOwnProperty('_internalModel') === true or something?

The global require in this file was causing an error when using babel 6 with the transform-es2015-modules-amd plugin

fixes emberjs#4134
bmac added a commit that referenced this pull request Feb 12, 2016
@bmac bmac merged commit 45b36b5 into emberjs:master Feb 12, 2016
@bmac
Copy link
Member

bmac commented Feb 12, 2016

Thanks @jmurphyau.

@sly7-7
Copy link
Contributor

sly7-7 commented Feb 12, 2016

I don't know if this matters here, but it seems to me we should avoid using hasOwnProperty() because it is slow. Maybe here it's not a hot path, so I guess it's ok.

@pangratz
Copy link
Member

Since this will be stripped in production builds I think this is ok...

@sly7-7
Copy link
Contributor

sly7-7 commented Feb 12, 2016

You're completely right

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

Successfully merging this pull request may close these issues.

relationships/has-many.js doesn't compile with Babel 6
6 participants