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

Make got an external dependency for the Node build target #15

Conversation

joshlory
Copy link

@joshlory joshlory commented Aug 4, 2017

Resolves

Issue #14.

Proposed Changes

Move got from a "devDependency" to a "dependency". Mark it as external for the Node.js webpack build.

Reason for Changes

Making got an explicit dependency lets the consuming project control whether got is built for a node or web target.

Test Coverage

I struggled a bit getting solid test coverage of this, or even a standalone repro case. It only happens when for import Storage from 'scratch-storage' is used in a client-side project.

I wrote a sample test case to verify gzipped assets can be loaded, but npm test runs via Node.js it doesn't repro the issue 😕. Happy to add appropriate coverage if you know of a better approach!

const crypto = require('crypto');
const test = require('tap').test;
const zlib = require('zlib');
const http = require('http');

global.self = {};
require('../../dist/web/scratch-storage');

const port = 8071;
const svgData = `<?xml version="1.0"?>
<svg width="100" height="100" xmlns="http://www.w3.org/2000/svg">
  <circle fill="#F00" cx="50" cy="50" r="30"/>
</svg>
`;

test('loadGzippedAsset', t => {
    const server = http.createServer((request, response) => {
        const raw = zlib.gzipSync(svgData);
        response.writeHead(200, {'Content-Encoding': 'gzip'});
        response.end(raw);
    }).listen(port);

    const storage = new self.Scratch.Storage();
    storage.addWebSource(
        [storage.AssetType.ImageVector],
        () => `http://localhost:${port}`
    );

    const assetType = storage.AssetType.ImageVector;
    const id = '8e768a5a5a01891b05c01c9ca15eb6aa';

    const promise = storage.load(assetType, id);
    t.type(promise, 'Promise');

    promise.then(asset => {
        t.type(asset, storage.Asset);
        t.strictEqual(asset.assetType, assetType);
        t.strictEqual(asset.assetId, id);

        const hash = crypto.createHash('md5');
        hash.update(asset.data);
        t.strictEqual(hash.digest('hex'), id);

        server.close();
    });

    return promise;
});

@joshlory
Copy link
Author

joshlory commented Aug 4, 2017

(I think this issue goes away if scratch-storage switches to nets along with scratch-vm, see: scratchfoundation/scratch-vm#585)

@thisandagain
Copy link
Contributor

Thoughts on this @rschamp @cwillisf? I think I'd rather resolve this issue by removing got and normalizing on use of the nets package.

@thisandagain thisandagain modified the milestones: Aug 23, Sept 20 Aug 10, 2017
@rschamp
Copy link
Contributor

rschamp commented Aug 10, 2017

+1 to moving to nets.

@joshlory
Copy link
Author

Closing this PR. Thanks!

@joshlory joshlory closed this Aug 10, 2017
@joshlory joshlory deleted the make-got-external-dependency branch August 10, 2017 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants