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

Improvements #13

Merged
merged 14 commits into from
Feb 11, 2019
Merged

Improvements #13

merged 14 commits into from
Feb 11, 2019

Conversation

TheAifam5
Copy link
Collaborator

@TheAifam5 TheAifam5 commented Feb 10, 2019

This pull request removes babel completely and uses tsc and uglifyjs only.
Adds also linting before commiting and prettier integration.
Also I switched to yarn and added missing package typescript.

I could add full webpack integration but for that simple code, you dont need it.

That's how the package looks like after (If I forgot some files, please tell me):

npm notice 📦  react-use-localstorage@2.4.0
npm notice === Tarball Contents === 
npm notice 1.4kB package.json  
npm notice 1.1kB LICENSE       
npm notice 3.8kB README.md     
npm notice 120B  lib/index.d.ts
npm notice 598B  lib/index.js  
npm notice === Tarball Details === 
npm notice name:          react-use-localstorage                  
npm notice version:       2.4.0                                   
npm notice filename:      react-use-localstorage-2.4.0.tgz        
npm notice package size:  3.2 kB                                  
npm notice unpacked size: 7.0 kB                                  
npm notice shasum:        c7c7b429afe66a01ed91370065da2d45f9f4952d
npm notice integrity:     sha512-PUOhUhaGfacMx[...]P5QCpwh6IyYEQ==
npm notice total files:   5

Also UglifyJS has more features if you need like manging, compressing etc.
Here is a example after compression enabled:

"use strict";var __importDefault=this&&this.__importDefault||function(mod){return mod&&mod.__esModule?mod:{default:mod}};Object.defineProperty(exports,"__esModule",{value:!0});var react_1=__importDefault(require("react"));function useLocalStorage(key,initialValue){void 0===initialValue&&(initialValue="");var _a=react_1.default.useState(function(){var value=localStorage.getItem(key)||initialValue;return localStorage.setItem(key,value),value}),item=_a[0],setValue=_a[1];return[item,function(newValue){setValue(newValue),window.localStorage.setItem(key,newValue)}]}exports.default=useLocalStorage;

The declaration file looks like this now:

import { Dispatch } from 'react';
export default function useLocalStorage(key: string, initialValue?: string): [string, Dispatch<string>];

Do not merge this yet :)

@TheAifam5
Copy link
Collaborator Author

That can be merged now if you agree.

@dance2die
Copy link
Owner

dance2die commented Feb 11, 2019

Thank you @TheAifam5.

Nice touch with Dispatch type 👍

I have played around with this PR and looks good & will merge.
(And learned a good flow [dropping babel]).

Using husky for the pre-commit hook idea was awesome
(I just learned of it from Kent C. Dodds' video on Egghead yesterday) 🙂.

As a side question,
May I ask about the benefit of using yarn over npm?

@dance2die dance2die merged commit 1a71d5a into dance2die:master Feb 11, 2019
@dance2die
Copy link
Owner

@allcontributors please add @TheAifam5 for code, infra

@allcontributors
Copy link
Contributor

@dance2die

I've put up a pull request to add @TheAifam5! 🎉

@dance2die
Copy link
Owner

@TheAifam5 I've bumped the major version as Dispatch<string> is a breaking change.
https://www.npmjs.com/package/react-use-localstorage/v/3.0.0

@TheAifam5
Copy link
Collaborator Author

TheAifam5 commented Feb 11, 2019

So IMHO yarn can handle dependecies better than npm. The folder is much smaller and Yarn has a lot of features like “resolution” or “workspaces” if you will plan do a monorepo with multiple packages.
Mainly the reason of switching to Yarn was the performance. There is also NPX but I didn’t tested it yet.
Npm since I last used it has a lot of features too, some of them are just ripped of from yarn.

If you dont like it, just a simple ‘npm install’ command and remove yarn.lock file.

@TheAifam5
Copy link
Collaborator Author

TheAifam5 commented Feb 11, 2019

Babel is really nice and you can integrate it with Typescript code just by installing preset @babel/preset-typescript. The one issue I see there, does not generate .d.ts files.

That could also be solved by:
Using babel with Typescript preset.
Copy index.ts to output for Typescript users.

Publishing the source code in package allows the user to decide how the package should be transpiled, for JavaScript users there would be pretranspiled JS file already available.

@dance2die
Copy link
Owner

Thanks for the great explanation @TheAifam5 👍. Learned much 🙂

For the .d.ts file the previous implementation generated one using tsc while .ts files were transipiled using babel, which I didn't think was ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants