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

Support exporting live-bindings from other chunks or external dependencies #2765

Merged
merged 2 commits into from Mar 25, 2019

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Mar 21, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Resolves #2754

Description

With this PR, re-exported variables will be updated correctly even if the live binding is exported from another chunk or an external dependency. This is done similar to how Babel handles this by using getters. This will work for all formats that did not support this before, i.e. 'cjs', 'amd', 'iife' and 'umd', and also work for star re-exports. Examples:

// input
export { x } from 'external';

// CJS output
'use strict';

Object.defineProperty(exports, '__esModule', { value: true });

var external = require('external');

Object.defineProperty(exports, 'x', {
	enumerable: true,
	get: function () {
		return external.x;
	}
});

// input
export * from 'external';

// CJS output
'use strict';

Object.defineProperty(exports, '__esModule', { value: true });

var external = require('external');

Object.keys(external).forEach(function (key) {
	Object.defineProperty(exports, key, {
		enumerable: true,
		get: function () {
			return external[key];
		}
	});
});

The only unsupported case for now is mutable default exports as at the moment, this would mean that Rollup needs to re-run its interop each time the getter is accessed. I hope to find a better solution for this in the future but this might require rethinking how interop works at the moment.

For cross-chunk exports, the new logic will only be used if the binding is actually mutated, otherwise a simple assignment is used. For external reexports, however, we always assume the binding is mutated.

@lukastaegert lukastaegert merged commit e53c734 into master Mar 25, 2019
@lukastaegert lukastaegert deleted the gh-2754-forward-live-binding branch March 25, 2019 05:40
@benjamn
Copy link

benjamn commented May 24, 2019

By switching to Object.defineProperty instead of assigning directly to exports (which I understand and support), this change seems to have caused problems with other conflicting (non-export * from) exports: apollographql/apollo-client#4843 (comment)

I'm happy to open an issue, but I wanted to check here first in case I'm missing something obvious?

@lukastaegert
Copy link
Member Author

Looking at the Apollo issue this looks like a bug to me. I also must admit I was not aware that export from excluded the default, which is a shame considering what I am working at. Please open an issue!

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.

Live bindings don't work in some cases
2 participants