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

Consider renaming didSubscribe to didMount #9

Open
develomark opened this issue Jun 5, 2018 · 3 comments
Open

Consider renaming didSubscribe to didMount #9

develomark opened this issue Jun 5, 2018 · 3 comments
Labels
good first issue Good for newcomers Hacktoberfest help wanted Extra attention is needed

Comments

@develomark
Copy link

Brilliant package. I really like this.

Please consider renaming didSubscribe to didMount so that it is crystal clear what this is doing.

I'm not sure it is clear immediately to users how this aligns to React or Vue. The fact that didSubscribe isn't handled server side means that 'For handling side-effects upon first subscription.' isn't true in every circumstance.

The added advantage is that new developers who have not seen proppy, but have used recompose should immediately understand what 'didMount' does.

@fahad19
Copy link
Owner

fahad19 commented Jun 5, 2018

Hi @develomark!

Thanks for the positive feedback!

I understand didMount sounds much more convincing to UI developers when they are using Proppy with React or Vue.

The reasonings behind naming it didSubscribe was to stay as close to the API of Proppy itself, which allows subscribing to it from the instance:

const unsubscribe = p.subscribe(props => console.log(props));

To make things easier for anyone using this library, I propose having an alias for didSubscribe, and call it didMount like you suggested.

So the package would export both the functions, but didMount would reference didSubscribe:

import { didSubscribe, didMount } from 'proppy');

// didSubscribe is same as didMount

@develomark
Copy link
Author

That makes sense to me.

Thank you for your response and feedback.

@fahad19 fahad19 added help wanted Extra attention is needed good first issue Good for newcomers labels Jun 11, 2018
@fahad19
Copy link
Owner

fahad19 commented Jun 11, 2018

When the issue is picked up, the following need to be done:

1: Export didMount

// packages/proppy/src/index.ts
import { didSubscribe } from './didSubscribe';

export const didSubscribe = didSubscribe;
export const didMount = didSubscribe;

2: Create new file for individual import

// packages/proppy/didMount.js
const didSubscribe = require('./lib/didSubscribe').didSubscribe;
module.exports = {
  didMount: didSubscribe,
};

3: Update docs

  • Add it to packages/proppy/README.md
  • Add it to site/content/api.md

4: Update examples

In examples/react-playground, mention didSubscribe and didMount together in the select field: https://codesandbox.io/s/github/fahad19/proppy/tree/master/examples/react-playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants