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

Revamp Build Infra for tiny-lru #27

Merged
merged 8 commits into from
Sep 27, 2019
Merged

Conversation

osdevisnot
Copy link
Contributor

@osdevisnot osdevisnot commented Sep 26, 2019

I feel like this PR might be controversial as its attempting to change lot of things. But I will try to present this as best as I can.

I started looking at tiny-lru as it is a dependency of graphql-hooks-memcache while investigating #366. It seems graphql-hooks-memcache is trying to use tiny-lru as es6 dependency, and es6 exports from tiny-lru isn't exactly consumable.

Incidentally, I recently started working on tslib-cli which attempts to streamline tooling/setup required for publishing libraries to NPM. In short, it provides:

OcJS - Rollup + Jest + Typescript + TSLint + Prettier
Best Practices setup for build, format, lint and publish workflows
Save Hours setting up tooling to release a library to NPM.

With these in mind, I started porting tiny-lru to use tslib-cli for build tooling. This PR presents the list of changes needed for such port.

Looking into it more, we can even extend this PR to auto generate type definitions, which will need us to port code from js to ts. It might also need some work on eslint to tslint migration if we decide to port code to ts.

NB: I will use comments on diff to explain some of the changes in this PR as this description is getting rather long.

Thanks in advance for looking into this ❤️

Output of npm pack after these changes:

image

output file sizes after these changes:
image

.idea/*
node_modules
.idea
*.tgz
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignore the removal of star, this mainly adds tgz to ignore list.

.travis.yml
Gruntfile.js
benchmark.js
bower.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better to use whitelist as opposed to maintaining whitelist to ignore files during publish. See https://gist.github.com/ceejbot/610773f9990a751db68dc6c66974b8ec#whitelisting-files-with-the-files-array

Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't work well IRL, as it'll include a CHANGELOG if present

Copy link
Owner

Choose a reason for hiding this comment

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

nm, it's always included 👍

grunt.registerTask("test", ["eslint", "nodeunit"]);
grunt.registerTask("build", ["copy", "concat"]);
grunt.registerTask("default", ["build", "test", "babili"]);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is little opinionated. grunt works fine here, but I feel like it's a lot of config work for such a small library. I mean when the grunt file is larger than the code, we should probably avoid it.

public keys(): string[];
}
export default Lru;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems this was just copied from root to lib, which looks un-necessary. The type file at root works perfectly too if package.json has types key.

@@ -1,3 +0,0 @@
"use strict";

(function (global) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file is not needed anymore.

} else {
global.lru = factory;
}
}(typeof window !== "undefined" ? window : global));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file is not needed anymore. the factory function is included in main file src/lru.js

"allowJs": true,
"sourceMap": true
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a minimal typescript configuration file

}

return new LRU(max, ttl);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was moved here from src/outro.js

"grunt-contrib-copy": "^1.0.0",
"grunt-contrib-nodeunit": "^2.0.0",
"grunt-contrib-watch": "^1.1.0",
"grunt-eslint": "^22.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove all babel and grunt related dependencies.
add nodeunit and tslib-cli as dependencies.

"browser": "lib/tiny-lru.js",
"main": "lib/tiny-lru.cjs.js",
"module": "lib/tiny-lru.esm.js",
"types": "tiny-lru.d.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so essentially

  • we want nodejs code main property to read lib/tiny-lru.cjs.js which was lib/tiny-lru.js before.

  • we are changing types to just point at file at root and avoid copy to lib.

  • we are also renaming module entry point from lib/tiny-lru.es6.js to lib/tiny-lru.esm.js -- This is to avoid versioning confusion in future. Releasing es2020 code for example won't need to change this file again in future.

  • we are introducing a new browser property to publish our modules as umd -- read here about browser property

  • we are also removing un-minified version (we minify by default). I am planning to release a new version of tslib-cli which will start producing sourcemaps too, which is not available for now.

@avoidwork
Copy link
Owner

Looks good to me.

@avoidwork avoidwork merged commit 6641496 into avoidwork:master Sep 27, 2019
@avoidwork
Copy link
Owner

What would you categorize this as? major or minor semver change? i'm thinking minor.

@osdevisnot
Copy link
Contributor Author

Considering that we are changing output file names, it might impact someone relying on file paths for imports. Specifically the who were following previous webpack usage guidelines in readme.

That said I would consider this as a major version

@avoidwork
Copy link
Owner

👍

@avoidwork
Copy link
Owner

I'm totally gonna copy/pasta this around some of my other repos.

@osdevisnot
Copy link
Contributor Author

I'm totally gonna copy/pasta this around some of my other repos.

I have WIP docs here: https://tslib-cli.js.org/ in case you need to refer back. Feel free to let me know if you ever need details on specific parts of tslib-cli

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

2 participants