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

fix: commonjs: empty export when setter and defineProperty both used #1447

Closed
wants to merge 2 commits into from

Conversation

coderaiser
Copy link

@coderaiser coderaiser commented Feb 27, 2023

Rollup Plugin Name: {Commonjs}

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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Fixes:

When setter used with defineProperty would be better to use getter in return inside of variable received by getter:

before:

var ansiStylesExports = {};
var ansiStyles = {
  get exports(){ return ansiStylesExports; },
  set exports(v){ ansiStylesExports = v; },
};

function requireAnsiStyles () {
	if (hasRequiredAnsiStyles) return ansiStylesExports;
	hasRequiredAnsiStyles = 1;
	(function (module) {
		function assembleStyles() {
			const styles = {
			};

			return styles;
		}

		// Make the export immutable
		Object.defineProperty(module, 'exports', {
			enumerable: true,
			get: assembleStyles
		});
} (ansiStyles));
        // empty object
	return ansiStylesExports;
}

after:

var ansiStylesExports = {};
var ansiStyles = {
  get exports(){ return ansiStylesExports; },
  set exports(v){ ansiStylesExports = v; },
};

function requireAnsiStyles () {
	if (hasRequiredAnsiStyles) return ansiStylesExports;
	hasRequiredAnsiStyles = 1;
	(function (module) {
		function assembleStyles() {
			const styles = {
			};

			return styles;
		}

		// Make the export immutable
		Object.defineProperty(module, 'exports', {
			enumerable: true,
			get: assembleStyles
		});
} (ansiStyles));
        // correct result
	return ansiStyles.exports;
}

@lukastaegert
Copy link
Member

lukastaegert commented Feb 27, 2023

While this fixes the mentioned issues, it will also break #1306 again by partially reverting #1363. And in a bad way: It keeps the inefficient weird getter/setter syntax while still relying on syntheticNamedExports.
I already started work on a proper fix for this some time ago that will go back to using the old object form while avoiding the reliance on syntheticNamedExports. If there is a lot of pressure to fix the issues, then please revert #1363 properly, knowing that we need to reopen #1306, otherwise wait until I completed my fix.

@coderaiser
Copy link
Author

coderaiser commented Feb 28, 2023

When I write code this way:

  • no syntheticNamedExports;
  • no weird getter/setter syntax;
  • go back to using the old object form

image

I have an runtime error:

image

It's related to to-fast-properties

@shellscape
Copy link
Collaborator

Citing @lukastaegert we're going to close this one. #1455 should address this when merged.

@shellscape shellscape closed this Apr 4, 2023
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.

None yet

3 participants