-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update blueprints to use native JS classes #17621
Conversation
@@ -13,6 +13,7 @@ const expect = chai.expect; | |||
|
|||
const generateFakePackageManifest = require('../helpers/generate-fake-package-manifest'); | |||
const enableModuleUnification = require('../helpers/module-unification').enableModuleUnification; | |||
const enableOctane = require('../helpers/setup-test-environment').enableOctane; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend to include the enableModuleUnification
function into the /helpers/setup-test-environment
module, but I would like to do that in an independent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one minor suggested tweak.
blueprints/native-detector.js
Outdated
|
||
module.exports = function(blueprint) { | ||
blueprint.filesPath = function() { | ||
let rootPath = process.env.EMBER_VERSION === 'OCTANE' ? 'native-files' : 'files'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, we intend to come up with a better "flag" than process.env.EMBER_VERSION
, but this unlocks actually being able to do the blueprint work while we figure that out.
3f1ddf8
to
698e8b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for working on this!
@rwjblue, I have included the other blueprints and this is ready for merging now. Do we have also to support Native classes in the Ember Data blueprints? ember-cli/ember-octane-blueprint#37 is related to this PR in case you want to share your opinion on how/if to get rid of the |
|
||
module.exports = function(blueprint) { | ||
blueprint.filesPath = function() { | ||
let rootPath = process.env.EMBER_VERSION === 'OCTANE' ? 'native-files' : 'files'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't this edition in the blueprint? Am I misremembering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the question.
Here is checking if the ENV.EMBER_VERSION
is equal to 'OCTANEto use Native files instead of Ember
extend`.
ember-cli/ember-octane-blueprint#36 set the env variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, you right. I was misremembering. thank you! :)
Implements
Update blueprints for each object type to use native JS classes
of #17234Related PRs ember-cli/ember-octane-blueprint#36 and ember-cli/ember-octane-blueprint#37