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

Change exports usage #230

Merged
merged 1 commit into from Oct 14, 2017
Merged

Change exports usage #230

merged 1 commit into from Oct 14, 2017

Conversation

btd
Copy link
Contributor

@btd btd commented Oct 13, 2017

Hello.

free exports usage (when it is not assignmnet for really exporting something) confusing rollup to wrap such modules in additional wrappers.

This PR remove exports for self referencing exporting symbols.

Thanks.

What do you think about replacing build with rollup (i mean dist/qs.js) - it will produce smaller file, and much readable/debuggable? (i can prepare PR)

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'm not going to use rollup; it violates the spec. Similarly, if it can't understand exports. syntax - a CJS standard for years - that's a pretty massive flaw with it.

That said, using exports is definitely a bad practice anyways, and this is a good change. Thanks!

@ljharb ljharb merged commit 6f0586f into ljharb:master Oct 14, 2017
@btd
Copy link
Contributor Author

btd commented Oct 14, 2017

Yep, i understand what you mean. Thank you for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants