-
Notifications
You must be signed in to change notification settings - Fork 148
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
Rollup v2 #225
base: master
Are you sure you want to change the base?
Rollup v2 #225
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hath995, thanks again for the PR and the great description! IIUC the main issue preventing you from using rollup
is the crypto
package. There are a few other things but I believe their impact is minor (e.g. ~10% increase in bundle size). If so, I think there is a better path forward than merging this PR:
- Switching from
avsc
to@avro/types
. This should be a straightforward port fromavsc
if you are only using the types part of it. Two main differences are that it doesn't include afingerprint
method on types (which lets us remove thecrypto
dependency) and thefrom/toString
methods are now replaced by the more naturalfrom/toJSON
. - Requiring this updated module and bundling it from your code. In case it's helpful, I uploaded a working example gist.
Let me know what you think.
@@ -99,17 +97,17 @@ function Type(schema, opts) { | |||
name = qualify(name, namespace); | |||
if (isPrimitive(name)) { | |||
// Avro doesn't allow redefining primitive names. | |||
throw new Error(f('cannot rename primitive type: %j', name)); | |||
throw new Error(`cannot rename primitive type: ${JSON.stringify(name)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you changed this to remove the dependency on util
, however I don't think the downsides--extra dependency (inherits
) and loss of compatibility with node <1
--are worth the benefit: the bundle size drops only by 10%, from 78kB to 70kB (23kB to 20kB after gzip
). It also seems relatively likely that clients would depend on util
for other things, in which case this change could actually make their bundle size go up (they end up having to include util
and inherits
).
return hash.read(); | ||
function getHash(str) { | ||
var hash = new md5(); | ||
return Buffer.from(hash.hex(str), "hex"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to a comment above, this requires node 4
. (utils
has a simple workaround for this one: utils.bufferFrom
.)
@@ -0,0 +1,29 @@ | |||
const rollup = require('rollup'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to move this logic outside of the core package, either in a dedicated one (with a peer dependency on @avro/types
) or in client code:
- It reduces bloat in the main package; most clients don't use
rollup
. - It allows clients to configure
rollup
options (e.g. they might not want to useterser
). - IIUC there are also benefits to rolling up more packages together, so it's counter-productive to bundle just this one separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never meant to actually commit this file (or the updated dependencies in package.json). This was there so that you could try to run the config I was using. I planned on removing it for any final pr.
@@ -446,7 +444,7 @@ Type.isType = function (/* any, [prefix] ... */) { | |||
}; | |||
|
|||
Type.__reset = function (size) { | |||
debug('resetting type buffer to %d', size); | |||
//debug('resetting type buffer to %d', size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed these because 'debug' was another node library that I didn't want to include in my bundle if I could avoid it. There were more bundle problems with it there so I removed it.
I think I misunderstood why you recommended @avro/types initially. It is closer to what I wanted but it had the same problem with rollup issues and also the TypeScript types are not included in the distribution of @avro/types. My entire project (client and server) is in TypeScript and so having the types is quite helpful. I wasn't familiar with Avro before trying out this library and I'm still experimenting if binary transport is a useful addition or not. The avro json schema description format was a bit confusing but thankfully the included TS types did eventually guide me to a correct description. I'm trying to marshal a recursive representation of a virtual DOM with a type similar to this. export interface VNode {
sel: string | undefined;
data: VNodeData | undefined;
children: Array<VNode | string> | undefined;
text: string | undefined;
key: Key | undefined;
}
export interface VNodeData {
props?: Props;
attrs?: Attrs;
class?: Classes;
style?: VNodeStyle;
[key: string]: any;
} https://jsperf.com/avsc-decode-coupon/12 It seems like on a large complicated data structure like this avsc takes a bit longer. It also only reduced the file size by ~26%, where as compression (with snappy) had a much bigger impact (nearly 80%). However snappy requires a buffer to either way I need to convert from json to a buffer or a encode it with avro. |
In sort of an unrelated question about union types in avro/avsc. I'm using a feature of typescript called Discriminated Unions heavily. Avro seems to support this. However, it seems I need to box these objects before avsc will encode them? Is it possible with some more effort to be able to encode the example interface IChangeMeta {
timeOffset: number;
updateId?: number;
}
type IPosition = {x: number, y: number};
type IMouseMoveEvent = { kind: "mouseMove", pos: IPosition } & IChangeMeta;
type IPushStateEvent = { kind: "pushstate", url: string } & IChangeMeta;
var reg = { registry: {}};
var MouseMoveAvro = avro.Type.forSchema({
name: "mouseMove",
type: "record",
fields: [
{name: "kind", type: {type: "enum", name: "mouseMoveEnum", symbols: ["mouseMove"]}},
{name: "pos", type: {name: "IPosition", type: "record", fields: [
{name: "x", type: "float"},
{name: "y", type: "float"}
]}},
{name: "updateId", type:"int"},
{ name: "timeOffset", type: "int" }
]
}, reg);
var PushStateAvro = avro.Type.forSchema({
name: "pushstate",
type: "record",
fields: [
{name: "kind", type: {type: "enum", name: "pushstateEnum", symbols: ["pushstate"]}},
{name:"url", type:"string"},
{name: "updateId", type:"int"},
{ name: "timeOffset", type: "int" }
]
},reg);
var updatesAvro = avro.Type.forSchema({
name: "updates",
type: "array",
items: ["pushstate","mouseMove"]
}, reg);
var data = [{
kind: 'pushstate',
url: 'https://www.yelp.com/search?find_desc=delivery&find_loc=East%20Solano%20Ave%2C%20Berkeley%2C%20CA&ytp_st=delivery_default&attrs=PlatformDelivery%2CRestaurantsPriceRange2.3%2CRestaurantsPriceRange2.2&l=p%3ACA%3ABerkeley%3A%3ADowntown_Berkeley&ed_attrs=RestaurantsPriceRange2.4',
updateId: 137,
timeOffset: 12350
},
{
kind: 'mouseMove',
updateId: 16,
pos: { x: 262, y: 152 },
timeOffset: 3490
}];
var data2 = [{ pushstate: data[0] }, { mouseMove: data[1] }]
updatesAvro.toBuffer(data2) |
Right, by default unions with multiple object types are wrapped:
There is no built-in utility but it is straightforward to do given how unions are encoded (Avro simply prefixes the branch's index). So, assuming you know the type of the serialized data, you can get the index of the branch it corresponds to, then use an |
JSON de/serialization tends to perform very well on data which is very string-heavy. I'm not sure why |
Following up from #224 this is the more minimal refactor. It did not improve the rolled up size much as I hoped (95k vs 68k but either way is better than browserify's 206K), however, this was sufficient that I could actually bundle it with my project. Now I'm unblocked to use avsc without making a separate browserify bundle.
One of the big issues was the crypto library. Apparently, it seems the rollup-plugin-node-builtins has trouble with the crypto module and I tried out md5.js but it had a different issue being packaged because of some circular dependency. Jshashes is entirely self contained and it was bundle-able.
Maybe someone else will find this useful.