Uh oh!
There was an error while loading. Please reload this page.
feat(makeDefaultExport): add deprecation message for generated default exports - #128
feat(makeDefaultExport): add deprecation message for generated default exports#128lennyburdette wants to merge 2 commits into
default exports#128Conversation
f43ec01 to
14e8348Compare| var instrumented = transform(source, { | ||
| plugins: ['transform-es2015-destructuring'] | ||
| plugins: [ |
There was a problem hiding this comment.
Can we extract the babel options to a shared variable?
| included: function() { | ||
| included: function(app, parentAddon) { | ||
| var isDevelopmentOrTest = app.env === 'development' || app.env === 'test'; |
There was a problem hiding this comment.
Can we refactor this to the following?
var isProduction = process.env.EMBER_ENV === 'production';
| var target = parentAddon || app; | ||
| target.options = target.options || {}; | ||
| target.options.loader = target.options.loader || { debug: isDevelopmentOrTest }; |
There was a problem hiding this comment.
I don't think we should add a config for this if we don't need to...
| prepend: true | ||
| }) | ||
| }); | ||
| } else if (target.options.loader.debug) { |
There was a problem hiding this comment.
If we tweak the above, this becomes } else if (!isProduction) {
| 'generated one anyway. This behavior is deprecated and will be removed in v5.0.0.'; | ||
| }; | ||
| Object.defineProperty(requirejs, 'deprecationLogger', { |
There was a problem hiding this comment.
Why do we need this? If it is just for the testing, can we tweak it to just mock console?
There was a problem hiding this comment.
I'm not entirely sure what you're getting at, but I simplified things a bit and changed the arguments to support setting loader.deprecationLogger = Ember.deprecate if we wanted. What do you think?
rwjblue
commented
Jul 26, 2017
Sorry for the delay in reviewing @lennyburdette |
trentmwillis
left a comment
There was a problem hiding this comment.
Should probably note in the deprecation message that this behavior can be turned off via loader.makeDefaultExport now.
…lt` exports If a module does not define a default export, loader.js assigns `exports.default = exports` for compatibility with modules that don't use `_interopRequireDefault` from babel6. Mutating exports is unexpected behavior and causes difficult-to-diagnose [bugs][1]. We should remove `makeDefaultExport` in version 5.0. This update * adds a deprecation warning in preparation for removing `makeDefaultExport`. * adds a debug build with the deprecation enabled. Addresses ember-cli#114 [1]:mike-north/ember-lodash#104
This change adds a `loader.debug` option to Ember CLI builds. It defaults to true for development and test builds. When true, it prepends loader.debug.js instead of loader.js to the application's vendor.js.
14e8348 to
fc83edbComparelennyburdette
commented
Aug 1, 2017
@trentmwillis Good catch. I added instructions for disabling makeDefaultExport in the deprecation message. |
lennyburdette
commented
Aug 1, 2017
@rwjblue thanks for the review! |
If a module does not define a default export, loader.js assigns
exports.default = exportsfor compatibility with modules that don't use_interopRequireDefaultfrom babel6. Mutating exports is unexpected behavior and causes difficult-to-diagnose bugs. We should removemakeDefaultExportin version 5.0. This updatemakeDefaultExport.Addresses #114