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

Do we really need `string.prototype.trim`? #444

Closed
shelacek opened this issue Sep 15, 2018 · 5 comments
Closed

Do we really need `string.prototype.trim`? #444

shelacek opened this issue Sep 15, 2018 · 5 comments

Comments

@shelacek
Copy link

@shelacek shelacek commented Sep 15, 2018

Package string.prototype.trim adds some not negligible cost into node_modules. According to http://npm.anvaka.com/#/view/2d/tape it is 8 packages:

  • define-properties
  • es-abstract
  • es-to-primitive
  • is-date-object
  • is-regex
  • is-symbol
  • object-keys
  • string.prototype.trim

which made up ~30% of the total size of node_modules:

$ du -bh --max-depth=1 node_modules/
27K     node_modules/is-symbol
7.7K    node_modules/inflight
21K     node_modules/through
19K     node_modules/is-date-object
37K     node_modules/minimatch
7.6K    node_modules/path-is-absolute
15K     node_modules/brace-expansion
36K     node_modules/object-keys
42K     node_modules/minimist
179K    node_modules/tape
18K     node_modules/resumer
45K     node_modules/es-to-primitive
8.0K    node_modules/once
59K     node_modules/glob
18K     node_modules/fs.realpath
25K     node_modules/is-regex
21K     node_modules/for-each
15K     node_modules/has
6.9K    node_modules/wrappy
17K     node_modules/concat-map
13K     node_modules/path-parse
28K     node_modules/string.prototype.trim
11K     node_modules/balanced-match
243K    node_modules/resolve
31K     node_modules/define-properties
7.8K    node_modules/inherits
34K     node_modules/is-callable
48K     node_modules/object-inspect
26K     node_modules/deep-equal
33K     node_modules/function-bind
17K     node_modules/defined
4.1K    node_modules/.bin
183K    node_modules/es-abstract
1.3M    node_modules/

It is not so important, but it is pretty much for a quite simple one liner, which is used only in one place.

Maybe something like this would be enough:

if (msg == null) throw new TypeError();
forEach(String(msg).split('\n'), function (aMsg) {
    that.emit('result', aMsg.replace(/^\s*#?\s*|\s+$/g, ''));
});
@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Sep 15, 2018

The size of node_modules is irrelevant; more deps is better; this is a test framework so bundle size is irrelevant; I’m not sure what benefit there would be from inlining it.

@shelacek

This comment has been minimized.

Copy link
Author

@shelacek shelacek commented Sep 15, 2018

Of course you are right. I don't have a problem with it. But I tend keep a look to my node_modules, because every dependency multiplies disproportionately. One of the reason I love about the tape is that it is relatively small compared to others.

http://www.youtube.com/watch?v=M3BM9TB-8yA&t=12m37s

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Sep 15, 2018

That talk is ridiculous - every complaint are part of the very reasons node is successful and widely used.

Thanks for filing the issue - it’s always a reasonable question to ask - but having deps is a good thing, and disk space is infinite and free, while cognitive overhead and complexity is expensive.

I’m going to close this, but we can keep discussing if you like!

@ljharb ljharb closed this Sep 15, 2018
@shelacek

This comment has been minimized.

Copy link
Author

@shelacek shelacek commented Sep 15, 2018

I'm not sure, that have deps on everything is good thing, but that's my opinion. Thank's @ljharb for your great work on this project, I appreciate it.

@ORESoftware

This comment was marked as disruptive content.

Copy link

@ORESoftware ORESoftware commented Sep 16, 2018

mmmm disk space is not infinite, and the fewer deps the better, but OK :)

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.