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

Refactoring Suggestions #11

Merged
merged 6 commits into from Mar 26, 2019

Conversation

Projects
None yet
2 participants
@nickytonline
Copy link
Contributor

commented Mar 26, 2019

Just some things you might want to consider.

@nickytonline nickytonline marked this pull request as ready for review Mar 26, 2019

@nickytonline

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Some refactor suggestions for #12.

},
"typeRoots": ["./types"],

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 26, 2019

Author Contributor

This is not necessary. By default, TypeScript will look in your node modules for all @types/* packages and include them.

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 26, 2019

Member

This is me being irresponsible & not knowing what each option does...

(I am sorry I resolved conversations thinking it was required to merge the PR...)

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 26, 2019

Author Contributor

No need to be sorry. 😉

This comment has been minimized.

Copy link
@dance2die

@dance2die dance2die merged commit 1e44e9f into cshooks:master Mar 26, 2019

2 checks passed

security/snyk - package.json (dance2die) No manifest changes detected
security/snyk - packages/useTrie/package.json (dance2die) No manifest changes detected
@dance2die

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Thank you so much Nick for taking time proving the great tips, and even doing a PR.

I've merged it and will release it

@dance2die

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@allcontributors[bot] please add @nickytonline for code, infra, test

@allcontributors

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@dance2die

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

@dance2die

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

I will release it later (unpublished 1.0.7) after adding some tests for the hooks as well 😄
(Learning from the failure)

v1 0 7 error
https://codesandbox.io/s/oz19414lz


/*
* @param {string} word A word to check if it exists in the trie
* @param {boolean} exactSearch Return true only if the exact word is stored
*/
has = (wordToSearch: string, exactSearch: boolean = true): boolean => {
has(wordToSearch: string, exactSearch: boolean = true): boolean {

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 27, 2019

Member

I don't know why but uglifyjs is not keeping the has method when the arrow method is not used... 🤔
I had to revert it back b9f1aec to build a minified version...

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 27, 2019

Member

v1.0.8 reflects that change.
https://www.npmjs.com/package/@cshooks/usetrie

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 27, 2019

Author Contributor

Weird. Maybe look at terser. See this thread https://twitter.com/dan_abramov/status/1060245441084825600 or just google terser.js

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 27, 2019

Member

It is a funny coincidence I was looking at terser just now 😄

I've used it once and forgot about it.
https://github.com/dance2die/calendar-dates/blob/master/rollup.config.js#L26

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 27, 2019

Member

From what I've read, https://iamturns.com/typescript-babel/
Babel Typescript is not perfect but as my code base isn't using any of new feature, I am thinking about going with Webpack || Rollup (to transpile as ES6 -> terser/minify -> babel to ES5)

Would it be too much for Webpack/Roll up? or should I keep it as arrow methods?

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 28, 2019

Member

Reducer looked OK though 🤔

function reducer(e, t) {
  switch (t.type) {
    case 'ADD':
      return (
        e.trie.add(t.payload.word),
        __assign({}, e, { trie: e.trie })
      );
    case 'REMOVE':
      return (
        e.trie.remove(t.payload.word),
        __assign({}, e, { trie: e.trie })
      );
    default:
      return e;
  }
}
function useTrie(e, t, r) {
  void 0 === t && (t = !0),
    void 0 === r &&
      (r = function(e) {
        return e;
      });
  var i = new Trie(e, t, r),
    n = __read(React.useReducer(reducer, { trie: i, word: '' }), 2),
    o = n[0],
    s = n[1];
  return __assign({}, o.trie, {
    add: function add(e) {
      s({ type: 'ADD', payload: { trie: i, word: e } });
    },
    remove: function remove(e) {
      s({ type: 'REMOVE', payload: { trie: i, word: e } });
    },
  });
}
(exports.Trie = Trie), (exports.default = useTrie);

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 28, 2019

Member

Wait a sec... Doesn't spread work on prototypes? Let me see...

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 28, 2019

Member

Ah ha!

Spreading in reducer didn't copy prototype.has.
prototype doesn't spread

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 28, 2019

Member

Spread works only on own & enumerable properties.
https://dmitripavlutin.com/object-rest-spread-properties-javascript/#12ownproperties

And MDN says propertype is not enumerable.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/prototype

I gotta blame my implementation of mutable Trie object instance & try to return a new object instance in useTrie...

This comment has been minimized.

Copy link
@dance2die

dance2die Mar 28, 2019

Member

I "might" convert the Trie class as immutable data structure.

This seems to help (requires Egghead subscription)
https://egghead.io/lessons/javascript-avoiding-mutations-in-javascript-with-immutable-data-structures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.