Closed darcyparker closed 3 years ago
Thank you, this helps, but I will need some time to figure things out.
NP - ask questions if you have any.
I have just contacted dprothero about taking over the npm!
Just pushed out a major update to JSONCrush that wraps it in a module, adds package.json, and is now updated on NPM!
Let me know if this is working ok for you now. Also it might be nice to provide typescript support. So let me know if there is an easy way to do that. Thanks!
It's nice to see it in npm now. I look forward to trying it out sometime.
If you want to add type definitions:
It looks like the new https://github.com/KilledByAPixel/JSONCrush/blob/master/JSONCrush.js is ES6 module format now.
Note: this pull request you closed wrote commonjs, es6 module and UMD module (browser) using rollup. Most people can use the es6 module these days... but you may wish to look at adding support for these in future.
BTW: You should add npm install jsoncrush
in your readme too.
thanks! i added the npm install instruction. Will c4a88c3 work with the new module setup or do I need to modify it? I don't know much about typescript at the moment.
This should work for JSONCrush.d.ts:
export type JSONCrush = {
crush(input: string, maxSubstringLength?: number): string;
uncrush(input: string): string;
};
But if you updated https://github.com/KilledByAPixel/JSONCrush/blob/master/JSONCrush.js and unwrapped the JSONCrush
object and export const crush = ...
and export const uncrush = ...
, it would be simpler. You can have 2 exports... and avoid the unnecessary wrapping these methods in another object. Then c4a88c3 would work as is.
You can also use an arrow fn. You don't need function
. (There is no this
so an arrow function is simpler.)
Great, thanks! I have added the ts now. I had thought about it and prefer a single export as that is how most libraries work. If I add more features eventually they will go in the JSONCrush object.
NP. (It's not a big deal to wrap it in an object. But think about it again... I think most libraries prior to ES6 modules wrapped things in an object because that object acted as a kind of namespace/module. But with an ES6 module, you can have separate exports and the file is the module. It is simpler to export things separately.)
Why not do both? For example, react
's default export is the React object that has createElement
, use*
hooks, createContext
, etc... as properties:
import React from 'react';
React.createElement('div');
but you can also import specific functions rather than the whole library:
import { createElement } from 'react';
createElement('div');
One advantage of only importing the functions that you plan to use is that the unused functions are not included in the bundle if dead code elimination is enabled, which is really nice if you like small bundles.
Because JSONCrush is not using export default
...
The way it is setup now, you have to import {JSONCrush} from 'jsoncrush'
.
export default
and separate export
would be my preference. This would help with the dead code elimination mentioned.
Because JSONCrush is not using
export default
...
Any reason that can't be done though?
The way it is setup now, you have to
import {JSONCrush} from 'jsoncrush'
.
I think it's kinda silly for a library to have a named export with its own name (ack. that it's pretty common, but I still think it's silly).
Ideally, you'd be able to use this library (or any library) like this:
import {crush, uncrush} from 'jsoncrush';
or
import JSONCrush from 'jsoncrush';
JSONCrush.crush(...);
JSONCrush.uncrush(...);
@antialias - I agree. That's what I was trying to politely say above when I said "But think about it again"
It should be an easy change for @KilledByAPixel if he wishes to. If not, I am glad this is in an npm module I can import.
Now using export default. Also updated ts to match, not 100% if it is correct, take a look if you get a chance.
https://github.com/KilledByAPixel/JSONCrush/blob/master/JSONCrush.d.ts
@KilledByAPixel - thanks for the change to the .js.
The JSONCrush.d.ts is not quite right. (It's not a class you can instantiate...) Try this:
declare const JSONCrush: {
crush(input: string, maxSubstringLength?: number): string,
uncrush(input: string): string
};
export default JSONCrush;
cool thanks!
Addresses https://github.com/KilledByAPixel/JSONCrush/issues/12
JSONCrush.js
source. There is still value in leavingJSONCrush.js
as is.rollup.config.js
is a very simple config for creating the 3 modules in thedist
folder..tmp
anddist
folders are in the.gitignore
and not managed source. The modules are built on install and put in thedist
folder.JSONCrush.js
into typescript like #2, I created a minimalJSONCrush.d.ts
that I think will satisfy most typescript users.types
property in thepackage.json
points toJSONCrush.d.ts
so typescript finds it automatically (if you use typescript)JSONCrush.d.ts
is a simple solution to help typescript users and keep it out of your .js code.Additional notes on
package.json
:name
isJSONCrush
. Note the case! This is different thanjsoncrush
in https://github.com/dprothero/JSONCrush/blob/master/package.json#L2.JSONCrush
and notjsoncrush
JSONCrush
in the npm registry. I think it would be a good idea, but if you don't, users can install withnpm install https://github.com/KilledByAPixel/JSONCrush.git
jsoncrush
and retire it... or update the repo it points to.JSONCrush
main
,module
, andbrowser
properties point to the Common JS, ES Module and UMD modules respectively. This lets the applicable JS environment pick the right source file for that environment.Before you approve/merge this pull request, you can test the install (and automatic builds of the 3 module formats):
Then with nodejs 15.x (and other versions that support ES Modules):
Or with earlier versions of node, you could do:
The objective of this pull request is just to create thin wrappers of
JSONCrush.js
to get them into the 3 module formats.