Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

Build web, node, and production bundle #16

Closed
wants to merge 15 commits into from
Closed

Build web, node, and production bundle #16

wants to merge 15 commits into from

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Mar 31, 2021

I've moved to Webpack and created two separate targets for node and dev.

Original PR: NebulousLabs/skynet-js#338
Closes #7

TODO (followup PRs)

Production build deploy

  1. The production web build should be deployed to our skynet-js HNS. I plan to include a deploy.js script in the repo which uses the Node version of the library itself to upload the production file and save the skylink to the registry. (Get the full, descriptive error response returned from skyd #351)

Update docs

  1. Remove nodejs-skynet SDK from the docs (and archive repo)
  2. Update nodejs examples
  3. Add notes about differences in API between Node and Web (since the former lacks File and the latter lacks access to the local filesystem)

Copy link
Collaborator

@kwypchlo kwypchlo left a comment

Choose a reason for hiding this comment

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

did a quick scan, looks quite good 👍

@@ -0,0 +1 @@
const { SkynetClient } = require("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this wasn't meant to be committed lol. The deploy script is a separate PR

Comment on lines +114 to +116
if (!portalUrl) {
portalUrl = defaultPortalUrl();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need that, you already have that in the argument list

Suggested change
if (!portalUrl) {
portalUrl = defaultPortalUrl();
}

const opts = { ...defaultDownloadOptions, ...this.customOptions, ...customOptions };
const url = this.getSkylinkUrl(skylinkUrl, opts);

window.open(url, "_blank");
Copy link
Collaborator

Choose a reason for hiding this comment

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

const url = this.getHnsUrl(domain, opts);

// Open the url in a new tab.
window.open(url, "_blank");
Copy link
Collaborator

Choose a reason for hiding this comment

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

same remark as with other window.open

Comment on lines +204 to +205
const merkleroot = response.data.merkleroot;
const bitfield = response.data.bitfield;
Copy link
Collaborator

Choose a reason for hiding this comment

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

optional tip: it's handy to use object destructuring assignment in such cases

Suggested change
const merkleroot = response.data.merkleroot;
const bitfield = response.data.bitfield;
const { merkleroot, bitfield } = response.data;

@mrcnski mrcnski marked this pull request as draft May 20, 2021 21:27
@mrcnski
Copy link
Contributor Author

mrcnski commented Apr 4, 2022

Closing. Don't think we'll ever get back to this, especially with skynet-js going into maintenance mode soon.

@mrcnski mrcnski closed this Apr 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build web, node, and production bundle
2 participants