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

[pkg] exports: use module.require for nodejs #3344

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
7 participants
@ichernev

This comment has been minimized.

Contributor

ichernev commented Aug 12, 2016

Ok the change looks good. Wow, we are causing a lot of trouble for webpack :)

@ichernev ichernev added this to the 2.15.0 milestone Aug 12, 2016

@afc163

This comment has been minimized.

Contributor

afc163 commented Aug 17, 2016

👍

@ichernev

This comment has been minimized.

Contributor

ichernev commented Sep 3, 2016

Merged in bb6a30e

@ichernev ichernev changed the title from use module.require for nodejs to [pkg] exports: use module.require for nodejs Sep 3, 2016

@ichernev ichernev closed this Sep 3, 2016

ichernev added a commit that referenced this pull request Sep 3, 2016

Merge pull request #3344 from yiminghe:module_require
[pkg] exports: use module.require for nodejs
@ljharb

This comment has been minimized.

ljharb commented Sep 14, 2016

um, this will break lots of tools - require is not the same as module.require.

@ljharb

This comment has been minimized.

ljharb commented Sep 14, 2016

Indeed, this PR caused a breaking change in a minor version (see #3431). Please revert it ASAP.

@yiminghe

This comment has been minimized.

Contributor

yiminghe commented Sep 14, 2016

if use require, webpack will load all locale data into browser(which is fine for nodejs but too much for web app).

This is a breaking change, but any serious web app should not depend on it.

@ljharb

This comment has been minimized.

ljharb commented Sep 14, 2016

If webpack breaks on the standard and normal require, that's a webpack bug, not one individual packages should solve.

@ljharb

This comment has been minimized.

ljharb commented Sep 14, 2016

Also, if it's a breaking change, it should be reverted for a v2.15.1, and published as v3.0.0 (at which point, Airbnb will no longer be able to use moment.js, so hopefully that doesn't happen)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Sep 14, 2016

@yiminghe can you give more details about your environment so we can work out a fix that works on both phantomjs/karma and your env (webpack?).

ichernev added a commit to ichernev/moment that referenced this pull request Sep 15, 2016

ichernev added a commit to ichernev/moment that referenced this pull request Sep 15, 2016

ichernev added a commit to ichernev/moment that referenced this pull request Sep 15, 2016

@yiminghe

This comment has been minimized.

Contributor

yiminghe commented Sep 18, 2016

@ichernev you can check the links in my pr. using require will cause browser to load all locales(too much for client) for webpack.

the idea usage(load on demand):

import 'moment/locale/zh-cn'; // import specified locale module
moment.locale('zh-cn');
@ichernev

This comment has been minimized.

Contributor

ichernev commented Sep 18, 2016

@yiminghe should I flip the checks order? Can you suggest something that works for the existing use case? So 'require' works for webpack but requires all of it, and if you use module.require it doesn't?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Sep 18, 2016

The whole idea of this code is to ONLY work on node.js. In other envs it's intended to be used by first requiring only the locales you want (in code) and then switching the locale -- the snippet you provided.

ichernev added a commit that referenced this pull request Sep 21, 2016

Revert "Merge pull request #3344 from yiminghe:module_require"
This reverts commit bb6a30e, reversing
changes made to d4c5f8e.

ichernev added a commit that referenced this pull request Sep 21, 2016

@ipa1981

This comment has been minimized.

ipa1981 commented Sep 21, 2016

Ok guys now it's reverted, so what's the plan now?

I use

import 'moment/locale/hr' # or require('moment/locale/hr'); the result is the same
moment.locale('hr')

and on version 2.15.1 it makes 150Kb heavier file (uglified) than on 2.5.0. So I had to revert to 2.5.0. 150K of dead weight is significant for web.

@ljharb

This comment has been minimized.

ljharb commented Sep 21, 2016

150k is a small-to-medium sized image.

It's definitely something that can be improved, but it sounds like a webpack bug. When I install moment locally and browserify a file that requires moment/locale/hr, the resulting file is 143K total with 2.15.1, and the identical 143K total with 2.15.0.

@ipa1981

This comment has been minimized.

ipa1981 commented Sep 23, 2016

@ljharb agree, 150KB could not be a problem in many site, but in high performance ones you fight for every KB. And there is additional 150KB of code browser needs to parse as well.

When it's considered as a webpack bug is it known for them?

@schmod

This comment has been minimized.

schmod commented Oct 12, 2016

This isn't a bug in Webpack. Webpack's noticing that Moment (potentially) depends on all of the locales, and bundles them. This is the correct and expected behavior, albeit an undesirable one for most Webpack users.

I'd propose queuing this change back up for the next release that will include breaking changes. It's a breaking change, but a sensible one.

@ljharb

This comment has been minimized.

ljharb commented Oct 12, 2016

In what way is it sensible? It breaks require for every tool that's not Webpack.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Oct 12, 2016

@schmod Wasn't there a way to tell webpack to bundle only manually specified files? I'm pretty sure a solution floated around.

If there is a way to trigger that behavior from whithin moment, that is great, or at least we can document it on momentjs.com.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Oct 12, 2016

Lets see : webpack/webpack#3128

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