Skip to content

Use nanocomponent 6#217

Merged
bcomnes merged 5 commits intomasterfrom
cc-nm
Jul 18, 2017
Merged

Use nanocomponent 6#217
bcomnes merged 5 commits intomasterfrom
cc-nm

Conversation

@bcomnes
Copy link
Copy Markdown
Contributor

@bcomnes bcomnes commented Jul 18, 2017

This makes the necessary changes to use the new nanocomponent. Farewell cache-component.

Copy link
Copy Markdown
Member

@ungoldman ungoldman left a comment

Choose a reason for hiding this comment

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

seems legit

Comment thread package.json
"musicmetadata": "^2.0.4",
"nanobus": "^4.1.0",
"nanocomponent": "^5.0.1",
"nanocomponent": "^6.0.0-0",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what's going on with the choo & nanocomponent release numbers? sub-patch releases now? 🤡

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Beta!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Believe it or not I didn't know about https://docs.npmjs.com/getting-started/using-tags

Anyway, we have a 'beta' release that you can install with nanocomponent@next and choo@next.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah nice

}

_update () {
return compare(arguments, this._args)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

o interesting 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good or bad interesting? this._args should probably be this.lastArgs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mainly just undocumented 😝

One thing I do continue to find odd is that all the class/prototype methods are underscore-prefixed which usually indicates private variables & methods that shouldn't be overridden, whereas these are in practice methods that are public and specifically meant to be overridden (or required to be defined in the case of _render).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Im at sea on the _s too. The idea is that anything with a _ aren't intended to be called on instances. If you want, take a pass at a nanocomponent's api or our components to come up with a better convention.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this._args could easily be made public at this.prevArgs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Bret Comnes Idea: Any property or method users interact with should not be _'d. Users implement .createElement and .update instead of _render and _update. .render stays as .render. We expose a few more properties e.g. this._args -> this.lastArgs
12:52 _hasWindow -> hasWindow
12:52 maybe
12:52 would make upgrading more painful
New messages
12:52 bret: oh, I like that!
12:52 bret: yeah; user implements createElement
12:52 woot!

What do you think @ungoldman ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bcomnes so what does createElement mean, and how is it different from render?

@bcomnes bcomnes merged commit 19bc23e into master Jul 18, 2017
@bcomnes bcomnes deleted the cc-nm branch July 18, 2017 02:53
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.

2 participants