Readme, license, build, linting, tests #1
Conversation
thomasdashney
changed the title
thomasdashney
force-pushed
the
build-and-test
branch
from
247ace0 to
1aea842
Compare
.eslintrc.json
Outdated
| "files": ["**/*.tsx", "**/*.js"] | ||
| } | ||
| ], | ||
| "ignorePatterns": ["dist/**/*.js", "dist/**/*.d.ts"], |
There was a problem hiding this comment.
Nit: could just ignore the whole folder
| "ignorePatterns": ["dist/**/*.js", "dist/**/*.d.ts"], | |
| "ignorePatterns": ["dist/**/*"], |
.eslintrc.json
Outdated
| "rules": { | ||
| "prettier/prettier": 1, | ||
| "@typescript-eslint/explicit-function-return-type": 0, | ||
| "@typescript-eslint/interface-name-prefix": [1, {"prefixWithI": "always"}], |
There was a problem hiding this comment.
I know we use this internally, but IMO its not the best practice for libs.
There was a problem hiding this comment.
Good call, agreed
| "dependencies": { | ||
| "eventemitter3": "4.0.0", | ||
| "hoist-non-react-statics": "^3.3.2", | ||
| "typesafe-actions": "4.4.2" |
There was a problem hiding this comment.
This version could probably be a lot less strict
| fs.copyFileSync(`${srcDir}/README.md`, `${destDir}/README.md`) | ||
| fs.copyFileSync(`${srcDir}/LICENSE`, `${destDir}/LICENSE`) |
There was a problem hiding this comment.
Any reason these need to be in the dist folder?
There was a problem hiding this comment.
For README.md, yes - I believe this is what shows up on the NPM page as the README (since the dist is what gets published).
For LICENSE - not necessarily, but I figure it doesn't hurt to include it here.
| @@ -0,0 +1,44 @@ | |||
| import resolve from '@rollup/plugin-node-resolve' | |||
There was a problem hiding this comment.
Might want to look at something like https://github.com/pikapkg/pack for bundling/distribution. Will eliminate a lot of the boilerplate and deals with creating all the various versions + types that consumers might want to use.
At the very least, any modern lib should expose an ES module, which is what we would be using.
There was a problem hiding this comment.
I actually gave it a shot but was getting weird errors and found it hard to debug. It's possible that some other changes I made afterwards in this PR would have fixed it, but I found it personally easier to use rollup since I've used it before.
I agree thought that Pika looks like it would take the maintenance weight off if.. definitely feel free to submit a PR if you get it running!
There was a problem hiding this comment.
Also, incase it was unclear, currently we have an ES tree-shakeable build via tsc: https://bundlephobia.com/[email protected]), as well as a CJS build via rollup if this were to be used in Node (which currently wouldn't work since we use DOM methods such as Headers, but will be helpful if we add SSR support)
|
@azmenak Merging to get this in so we can start using in the d1g1t UI, but feel free to submit a PR or comment on this if there's anything you want to improve. My next step is to tackle more of the README (#16), add CI and code coverage badge (#2), and submit a PR for smaller changes I want to add (#13, #14, #15). My goal is to get a 1.0.0 beta up sometime this week. |
What Changed
READMELICENSE"strict"mode and update code accordinglyBreaking changes:
restfulmixin toRestEndpointsclass (which extendsHttpEndpoints). This was necessary because Typescript was unable to generate definition file typings for the protected mixin methods (see Generating type definitions for mixin classes with protected members microsoft/TypeScript#17744)HttpEndpointsandRestEndpointsprivate & protected methods with an underscore (ie._get,_post,_list,_create). Non-typescript users won't see that these methods are protected, so the_prefix will help to indicate that these shouldn't be considered public.RestEndpoints'sdeletemethod todestroy. This avoids a naming collision withHttpEndpoints'sdeletemethod, which was preventing the ability to callHttpEndpoints'sdeletein a subclass ofRestEndpoints