search

NebulousLabs / nodejs-skynet

Library for integrating Skynet into Node.js applications
MIT License
29 stars 12 forks source link

Added "removeRootDir" option to directory uploads #32

Closed Delivator closed 4 years ago

Delivator commented 4 years ago

Added removeRootDir to DefaultUploadOptions which when enabled, strips the root directory from the filepaths on directory uploads. This is to have the same behavior as if we would upload using the webportal ui.

Eg. using skynet.UploadDirectory("./dist") would upload files to <skylink>/dist/<subfiles>, but when uploading directories using the web ui the path is <skylink>/<subfiles>.

So setting removeRootDir to true, replicates this behavior.

Delivator commented 4 years ago

Just like that?

mrcnski commented 4 years ago

@Delivator We can call it just once instead of twice. Can you do path = p.normalize(path) at the beginning of the function and remove the two later calls to normalize?

mrcnski commented 4 years ago

@Delivator I really like this option and I'll be adding it to the other SDKs. I'm thinking the option should be renamed though. People might confuse it with the Skynet root folder /var/skynet. I'm thinking removeBaseDir. Any ideas?

Delivator commented 4 years ago

removeBaseDir sounds great to me! 'Pushed those changes.

mrcnski commented 4 years ago

Hey @Delivator, we've been discussing this internally and we feel this should be the default behavior. It's only possible to upload one base directory at a time so the skylink should basically represent that directory. dir/file should be predictably represented at <skylink>/file. I think removeBaseDir can be removed and this just becomes the new behavior. This way our SDKs will also be consistent with the web interface. What do you think? 🙂

Delivator commented 4 years ago

Totally fine with this. I wasn't sure about compatibility but it makes sense to me!

mrcnski commented 4 years ago

It won't be compatible since developers might be making assumptions about the filepath in their code. We'll probably do a breaking change next time we publish.

Do you want to make this as the default behavior in this PR? Basically it just means removing the new variable.

Delivator commented 4 years ago

Like that https://github.com/NebulousLabs/nodejs-skynet/pull/32/commits/91d0df3308802d39298603ab5cd0f776ce288bca?

mrcnski commented 4 years ago

Exactly! I'll merge this after https://github.com/NebulousLabs/nodejs-skynet/pull/29 is accepted, thanks for your contribution.

Delivator commented 4 years ago

https://github.com/NebulousLabs/nodejs-skynet/pull/32/commits/08f60e0e90094e5b0a9d4244480bb19e72e7d864 broke all this again, on windows at least.

Because windows uses backslashes, in my example it now tries to remove dist/ from dist\js\chunk-vendors.36a8b4b3.js.map which obviously does nothing, as such the skylink inclues the full path again.

Delivator commented 4 years ago

35 should fix this again.