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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

2 cleaning up and syncing with py lib #10

Merged
merged 32 commits into from May 1, 2016

Conversation

vitorbaptista
Copy link
Contributor

@vitorbaptista vitorbaptista commented Feb 3, 2016

This is ready for review 馃憤

This gives us more confidence that the code runs both on NodeJS an on a browser
(at least on a WebKit browser).
We're using bluebird instead.
Apart from the obvious changes (becoming a class and such), the behaviour
changed in two other places:

* Instead of rejecting with a string, we throw an Error;
* We don't alter the csv.parse exceptions on error.
git-subtree-dir: lib/schemas
git-subtree-split: 1a06ab0a6ecab851e18f1a684e7f13a31db28821
This library needs to work both on the browser and on NodeJS, so we can't rely
on `fs.readFile`. Here we're using brfs (actually, babel-brfs because we need
ES6) to embed the default registry and its schemas into the compiled JS file.
Instead of embeding the cache into the JS files, we'll follow the
datapackage-registry-py pattern and load from the filesystem (when running in
NodeJS) or not have a cache (when in the browser).

This avoids issues with the schemas using `$ref` to point to other schemas, and
also will make the JS file smaller.

This reverts commit d4073c1.

Conflicts:
	lib/index.js
	test/test.js
It is also able to read files now. It'll be used when we're building the local
cache.
As the check is static, it doesn't need to be a function.
The behaviour in the browser is the same: load the default registry URL.
The profile is loaded from the local filesystem (if exists and we're on Node),
or from its URL. The base path is the Registry's base path in the local
filesystem, if it was loaded locally.

The base path is needed for loading schemas that use relative `$ref`s.
The ./src folder contains our code (ES6), the ./lib contains it compiled to
ES5, and the schemas cache was moved from ./lib/schemas to ./schemas.

In our tests, I've prefered to test the compiled ES5, instead of our ES6 code.
This means that the stacktraces will point to the compiled ES5 code, but if
they pass, we can be sure that the resulting package works (including the
compilation process).
We were only using it for the `.promisify()` method. Without it, the bundled
codebase goes from 546 KB to 398 KB. Still huge.
That's the only thing we're using anyway. This lowers our codebase from 398 KB
to 381 KB.
The RegExp isn't as good as what's done by URL, and we also consider
"file://.*" to be a remote URL, but the code went from 378 KB to 335 KB.
When requiring the polyfill, we're polluting the global scope, and making our
codebase bigger. They're not needed on many newer browsers, which might be all
that our users need to support. We can't make this decision for them, so we
better just document it.
@pwalsh
Copy link
Contributor

pwalsh commented Mar 8, 2016

Hey @vitorbaptista sorry for long delay on this :).

I did not think if this before, but part of the bloat is that we are including isomorphic-fetch and bluebird (or other promise lib) in our browser builds.

Is there a clean way that we could still use these as normal in the source, but ensure that the browser builds never bundle them? Then the instructions for users are that they need a browser with Promise and Fetch, or, they need to add a polyfill.

Putting aside the CSV parsing issue we've talked about, would this help with the bloat concerns for the lib?

@vitorbaptista
Copy link
Contributor Author

For Promise it's easy (as our NodeJS version already has Promises), for fetch it's a bit more tricky because NodeJS doesn't support it natively, so we would need a way to load isomorphic-fetch or node-fetch while on Node, but not bundle it for the browser.

That sounds like the way to go.

@rufuspollock
Copy link
Contributor

@pwalsh can this merge?

@pwalsh
Copy link
Contributor

pwalsh commented Apr 30, 2016

@rgrp yes. can you merge and publish? I was just holding to more deeply investigate pathways to reduce bloat in browser distributions of our core libs.

@rufuspollock rufuspollock merged commit f9df973 into master May 1, 2016
@pwalsh pwalsh deleted the 2-cleaning-up-and-syncing-with-py-lib branch May 1, 2016 21: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

3 participants