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

Avoid mutating module exports. #114

Open
rwjblue opened this issue Mar 5, 2017 · 4 comments
Open

Avoid mutating module exports. #114

rwjblue opened this issue Mar 5, 2017 · 4 comments

Comments

@rwjblue
Copy link
Member

rwjblue commented Mar 5, 2017

We should stop mutating the modules exports.

This was needed while ember-cli was using amdStrict mode (since it did not use the _interopRequireDefault babel helper), and that is being unrolled in ember-cli/ember-cli#6827.

Suggested migration path is:

  • Change makeDefaultExport to use Object.defineProperty(exports, 'default,...) with a getter that issues a deprecation warning in debug builds (avoiding Object.defineProperty in prod builds).
  • Remove deprecation and method after major version bump to [email protected].
@rwjblue
Copy link
Member Author

rwjblue commented May 12, 2017

This has reared its ugly head again. See description in mike-north/ember-lodash#104.

We need to move forward and kill makeDefaultExport. I am tired of wasting time debugging things in phantom due to this behavior. 😩

@lennyburdette
Copy link
Contributor

@rwjblue I'm happy to take this on. We (Square) are finally migrating to Ember CLI and we're trying to switch out our custom module runtime with loader.js ahead of time to discover any subtle incompatibilities. The cyclical default property is the first obvious issue.

One question:

What do you mean by "debug build"? Unless I'm missing something, there's only a prod build (dist/loader/loader.js) and an instrumented build (dist/loader/loader.instrument.js). Do we need to update build.js to output a loader.debug.js? And how would we get an Ember CLI build to use the debug build in development/test and the prod build in production?

The benchmarks load the instrumented or prod build conditionally, but I'm not seeing the instrumented build referenced elsewhere.

Thanks!

@lennyburdette
Copy link
Contributor

Ack: I just noticed the ember-addon code in index.js. I think I know how to proceed. But if you have any similar debug builds I could model this after, please let me know!

@rwjblue
Copy link
Member Author

rwjblue commented Jun 23, 2017

This library is now an ember-cli addon, we directly app.import the specific transpiled dist file that we need. We have a build script that we use to generate those pre-built assets.

Rough steps:

  • update babel version in use by the build script to 6
  • add babel-plugin-debug-macros
  • import debug flag and use if statements to guard the debug only code
  • update build script to emit debug, prod, instrumented debug, instrumented prod files
  • update included hook to app.import the "right" file

Happy to discuss further, feel free to ping me here or in the community slack...

lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
Before updating the build system to support stripping debug code, we need to upgrade to babel6.

Prep work for ember-cli#114
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
Upgrading to babel6 allows us to use the babel-plugin-debug-macros plugin in a future change.

Prep work for ember-cli#114
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
Upgrading to babel6 allows us to use the babel-plugin-debug-macros plugin in a future change.

Prep work for ember-cli#114
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
Upgrading to babel6 allows us to use the babel-plugin-debug-macros plugin in a future change.

Prep work for ember-cli#114
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
Upgrading to babel6 allows us to use the babel-plugin-debug-macros plugin in a future change.

Prep work for ember-cli#114
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
Upgrading to babel6 allows us to use the babel-plugin-debug-macros plugin in a future change.

Prep work for ember-cli#114
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
…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
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 23, 2017
…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
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 24, 2017
…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
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 24, 2017
…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
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Jun 24, 2017
Modules that return an object or only declare named exports are no longer mutated to include a `default` export.

Addresses ember-cli#114

Breaking change: loader.js will no longer create a default export.
mlunoe pushed a commit to mlunoe/loader.js that referenced this issue Jul 26, 2017
…port

This property allows for an immediate option to remove the functionality
that mutates the module exports (ember-cli#114)
and allows for a deprecation cycle when maintainers are ready to do so

Workaround for ember-cli#114
lennyburdette pushed a commit to lennyburdette/loader.js that referenced this issue Aug 1, 2017
…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
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

No branches or pull requests

2 participants