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

Added a new function and change some es6 features to es5 #86

Merged
merged 7 commits into from
Mar 27, 2018

Conversation

yoeven
Copy link
Contributor

@yoeven yoeven commented Mar 21, 2018

1. I've added getFileUploadLink which works like getFileDownloadLink. Similarly, it also allows users to use the request module to process the upload from their end.

A simple example:
var link = client.getFileUploadLink(RemoteDirToUploadTo + "/" + filename);

A simple example with the request and process-request modules:

    var data = fs.createReadStream(LocalDirFile);
    var options = {
        url: link,
        body: data,
    };

    var r =  request.put(options);

    progress(r, {})
        .on('error', function (err) {
            progressUpdateFunction("Error");
        })
        .on('end', function () {
            progressUpdateFunction("Done");
        });

    var amountdone= 0;
    data.on('data', function (data) {
        amountdone+= data.length;
        progressUpdateFunction((amountdone/ filesize) * 100);
    })

2. I've changed few es6 features to es5 features as react building process takes any node modules with es6 syntax as an error and breaks the build.

yoeven added 5 commits March 21, 2018 17:10
- getFileUploadLink function

- changed es6 syntax to es5 for electron-builder support
Copy link
Owner

@perry-mitchell perry-mitchell left a comment

Choose a reason for hiding this comment

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

Addition looks good, but could you please revert the ES5 changes? They're not required.

source/auth.js Outdated
@@ -4,10 +4,10 @@ function generateBasicAuthHeader(username, password) {
return "Basic " + Buffer.from(username + ":" + password).toString("base64");
}
function generateTokenAuthHeader(tokenInfo) {
return `${tokenInfo.token_type} ${tokenInfo.access_token}`;
return tokenInfo.token_type + " " + tokenInfo.access_token;
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this - Babel is used before publishing, so this will be transpiled. The tests pass for node 4, which is my earliest support version:

image

Anything outside of this is not currently in scope.

source/auth.js Outdated
generateBasicAuthHeader,
generateTokenAuthHeader
generateBasicAuthHeader: generateBasicAuthHeader,
generateTokenAuthHeader: generateTokenAuthHeader
Copy link
Owner

Choose a reason for hiding this comment

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

This can be reverted due to it not being required.

@@ -226,5 +239,5 @@ function createClient(remoteURL, username, password) {
}

module.exports = {
createClient
createClient: createClient
Copy link
Owner

Choose a reason for hiding this comment

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

Same here..

createReadStream,
createWriteStream
createReadStream: createReadStream,
createWriteStream: createWriteStream
Copy link
Owner

Choose a reason for hiding this comment

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

Same here..

@@ -70,5 +70,5 @@ function getDirectoryFiles(result, serverBasePath, requestPath) {
}

module.exports = {
getDirectoryContents
getDirectoryContents: getDirectoryContents
Copy link
Owner

Choose a reason for hiding this comment

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

And here..

@@ -36,7 +36,7 @@ function getFileLink(filePath, options) {
}
const authPart = options.headers.Authorization.replace(/^Basic /i, "").trim();
const authContents = Buffer.from(authPart, "base64").toString("utf8");
fetchURL = fetchURL.replace(/^https?:\/\//, `${protocol}://${authContents}@`);
fetchURL = fetchURL.replace(/^https?:\/\//, protocol + "://" + authContents + "@");
Copy link
Owner

Choose a reason for hiding this comment

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

And here 😊

throw new Error("Failed retrieving download link: Invalid authorisation method");
}
var authPart = options.headers.Authorization.replace(/^Basic /i, "").trim();
var authContents = Buffer.from(authPart, "base64").toString("utf8");
Copy link
Owner

Choose a reason for hiding this comment

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

Please use const/let (stylistic choice, to match rest of repo).

module.exports = {
putFileContents: putFileContents
putFileContents: putFileContents,
getFileUploadLink: getFileUploadLink
Copy link
Owner

Choose a reason for hiding this comment

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

Same here..

Copy link
Owner

Choose a reason for hiding this comment

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

@yoeven These can still be changed..

source/merge.js Outdated
@@ -5,5 +5,5 @@ function merge(...args) {
}

module.exports = {
merge
merge: merge
Copy link
Owner

Choose a reason for hiding this comment

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

And here..

@yoeven
Copy link
Contributor Author

yoeven commented Mar 24, 2018

Hi, @perry-mitchell I've reverted the es5 changes as stated. I seem to get a minifying file error from building a react project if I use the es6 features for this module. Based on few threads that I read online, it seems like es6 is an issue even though the oldest nodejs supports it (thread links here & here).

module.exports = {
putFileContents: putFileContents
putFileContents: putFileContents,
getFileUploadLink: getFileUploadLink
Copy link
Owner

Choose a reason for hiding this comment

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

@yoeven These can still be changed..

@perry-mitchell
Copy link
Owner

That sounds strange @yoeven - What version of node are you running? You can see here that webdav uses the built version of this library when required by other libraries. The code that is seen externally should be Node v4 compatible, so it shouldn't cause any issues. At this stage I would suggest that it's a configuration issue in your react app and not something to do with this library.

If you disagree or find an issue, definitely feel free to open a separate issue so we can debug it.

@yoeven
Copy link
Contributor Author

yoeven commented Mar 24, 2018

I'm using version 9.8.0 and Yeah, it could possibly be something with the configuration, I'll do a bit more research and if anything I'll post an issue. For now, we can stick with just the new function for this PR. Thanks @perry-mitchell

@perry-mitchell
Copy link
Owner

perry-mitchell commented Mar 24, 2018 via email

@perry-mitchell
Copy link
Owner

Just to remind: I'll merge after the latest changes are made. Thanks!

@perry-mitchell perry-mitchell merged commit 589ca32 into perry-mitchell:master Mar 27, 2018
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.

2 participants