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

Rename formats.default to formats.defaultFormat #229

Closed
wants to merge 1 commit into from
Closed

Rename formats.default to formats.defaultFormat #229

wants to merge 1 commit into from

Conversation

such
Copy link

@such such commented Oct 4, 2017

Having an attribute named 'default' confuses rollup when packaging.

See rollup/rollup#1135 for instance.

@such such changed the title Rename formats.default to format.defaultFormat Rename formats.default to formats.defaultFormat Oct 4, 2017
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with this change; this is a bug in rollup, since "default" is a valid property on the single export that a CJS module has.

If Rollup can't work with a perfectly valid CJS module, then I'd suggest you take that into consideration when deciding if rollup is a viable tool or not.

(it looks like you aren't based on latest master, however, as you have a lot of unrelated changes in this PR)

dist/qs.js Outdated
if (!chain.length) {
return val;
}
var parseObject = function (chain, val, options) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regardless, all of these changes where you removed function names would have to be reverted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand where this change comes from... My changes in lib are restricted to the object of this PR...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be rebased on a much older version of master?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, also, the dist file should never be updated except during a release :-) you can just revert this whole file.

@btd
Copy link
Contributor

btd commented Oct 13, 2017

iirc it is not an issue at all. rollup checks for __esModule variable. I created #230 to address what really confuse rollup.

@ljharb
Copy link
Owner

ljharb commented Oct 14, 2017

@btd you'll want to file a separate bug on rollup for that, then; a module bundler that doesn't understand a basic module format is broken.

I'm going to close this; tools should serve code, not the other way around.

@ljharb ljharb closed this Oct 14, 2017
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