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

Remove dependency on npm #1269

Merged
merged 2 commits into from
Feb 8, 2020
Merged

Remove dependency on npm #1269

merged 2 commits into from
Feb 8, 2020

Conversation

Hirse
Copy link
Contributor

@Hirse Hirse commented Jan 31, 2020

Currently, ungit has a dependency on the whole npm package, which is only used to check latest versions on the npm registry.

This change replaces npm with more light-weight solutions to provide the same functionality.

@campersau
Copy link
Collaborator

Quick review:

  • All resolved urls in the package lock now point to https://domoreexp.pkgs.visualstudio.com/_packaging/npm-mirror/npm/registry instead of https://registry.npmjs.org
  • I think pkg-versions can be a devDependency

Will look into it more during the weekend.

@Hirse
Copy link
Contributor Author

Hirse commented Jan 31, 2020

Fixed both of those.
I had messed up my registry from work so it was pointing to a mirror.

@campersau
Copy link
Collaborator

The build is currently failing because npm was previously required in sysinfo.js which did load graceful-fs which then replaced some fs functions and changed the behavior for having to specify callbacks.

We currently have a lot of fs calls for testing which don't specify a callback and maybe more:

ungit/source/git-api.js

Lines 699 to 720 in 621a04f

app.post(`${exports.pathPrefix}/testing/createfile`, ensureAuthenticated, (req, res) => {
const content = req.body.content ? req.body.content : (`test content\n${Math.random()}\n`);
fs.writeFileSync(req.body.file, content);
res.json({});
});
app.post(`${exports.pathPrefix}/testing/changefile`, ensureAuthenticated, (req, res) => {
const content = req.body.content ? req.body.content : (`test content\n${Math.random()}\n`);
fs.writeFileSync(req.body.file, content);
res.json({});
});
app.post(`${exports.pathPrefix}/testing/createimagefile`, ensureAuthenticated, (req, res) => {
fs.writeFile(req.body.file, 'png', { encoding: 'binary' });
res.json({});
});
app.post(`${exports.pathPrefix}/testing/changeimagefile`, ensureAuthenticated, (req, res) => {
fs.writeFile(req.body.file, 'png ~~', { encoding: 'binary' });
res.json({});
});
app.post(`${exports.pathPrefix}/testing/removefile`, ensureAuthenticated, (req, res) => {
fs.unlinkSync(req.body.file);
res.json({});
});

We could also change them to use their *Async versions instead because we promisify fs here
const fs = Bluebird.promisifyAll(require("fs"));

@campersau
Copy link
Collaborator

This will also reduce the electron package size by a few MB. (ungit-linux-x64.zip 98.9MB → 91.6MB in my environment.)

type: 'text',
additions: '3',
deletions: '0'
return common.get(req, '/status', { path: testDirMain })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch!

@campersau campersau merged commit 87261bf into FredrikNoren:master Feb 8, 2020
@Hirse Hirse deleted the npm-dep branch February 9, 2020 04:20
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

2 participants