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

Unused dependancy: brfs #45

Closed
simon-paris opened this issue Sep 8, 2016 · 16 comments
Closed

Unused dependancy: brfs #45

simon-paris opened this issue Sep 8, 2016 · 16 comments

Comments

@simon-paris
Copy link

Brfs is a very large module. Could it be moved to devDependancies if possible?

@devongovett
Copy link
Member

The problem is that it's required for browserify builds. Moving it to devDependencies would require the user to know to install brfs along with fontkit, or whatever required fontkit.

@jahewson
Copy link

jahewson commented Sep 8, 2016

Perhaps it could be moved topeerDependencies? That way npm will print a message.

@Pomax
Copy link
Contributor

Pomax commented Sep 8, 2016

note that if it's in devDependencies, it will still get automatically installed during an npm install run, unless the user explicitly runs npm install --production, which most people never do (but heroku etc. do)

@devongovett
Copy link
Member

@Pomax only if you are running npm install in the fontkit folder. If you are installing something that has fontkit as a dependency (e.g. npm install pdfkit), devDependencies won't be installed.

@Pomax
Copy link
Contributor

Pomax commented Sep 8, 2016

ah, right.

@devongovett
Copy link
Member

For brotli.js I managed to get rid of brfs by writing a base64 JS string version of the binary file and using require instead of fs.readFileSync. e.g. module.exports="base64";. Maybe we can do something similar here. Currently the only file we need to read is the Unicode trie for the arabic shaper.

@awerlang
Copy link

Currently, not only brfs but also browserify-optional, from this module and also in restructure, unicode-properties. It would chop away 40% of node_modules.

@mclark-newvistas
Copy link

mclark-newvistas commented Feb 21, 2019

It would also help fix the npm audit warnings about security vulnerabilities for pdfmake. See also bpampuch/pdfmake#1639.

@RoelN
Copy link

RoelN commented Feb 22, 2019

All this talk about chopping off large chunks of depedencies makes me happy. It'd speed up Wakamai Fondue considerably. E.g. I'd only need Brotli to decompress fonts, not to compress them.

In other words, +1

@devongovett
Copy link
Member

Unfortunately, all of those dependencies are still required. They are used for builds using browserify.

@Pomax
Copy link
Contributor

Pomax commented Feb 23, 2019

@RoelN what part would it speed up, though? The runtime should not be noticably affected by the dependency.

@ghost
Copy link

ghost commented Feb 24, 2019

Having real touble with this, npm install pdfkit --only=prod is kicking this back as a vulnerbility. From what I understand this shouldn't even be installed. Am I doing something wrong? Same issue on linebreak too.

@ghost
Copy link

ghost commented Feb 24, 2019

@Pomax only if you are running npm install in the fontkit folder. If you are installing something that has fontkit as a dependency (e.g. npm install pdfkit), devDependencies won't be installed.

@devongovett Is this statement strictly true? I thought it depended upon the value of NODE_ENV. Please see my previous comment

@awerlang
Copy link

@ghost
Copy link

ghost commented Feb 24, 2019

@awerlang thanks, I read that and tried several permutations but I am still seeing this included when installing the downstream package (pdfkit). This fails npm audit with a vulnerability due to brfs being a primary dependency of the fontkit package as has previously been debated.

@RoelN
Copy link

RoelN commented Feb 25, 2019

@Pomax I meant in package size, reducing download time. I don't expect any real difference in runtime.

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

No branches or pull requests

7 participants