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

enable lifecycleExperimental by default? #678

Closed
ljharb opened this issue Nov 12, 2016 · 10 comments
Closed

enable lifecycleExperimental by default? #678

ljharb opened this issue Nov 12, 2016 · 10 comments

Comments

@ljharb
Copy link
Member

ljharb commented Nov 12, 2016

#34 (comment) reminded me that perhaps for v3, we should enable this by default.

Any objections? @aweary @blainekasten @nfcampos @lelandrichardson

@aweary
Copy link
Collaborator

aweary commented Nov 12, 2016

Haha, I was just about to open this same issue. We haven't heard any reports of issues with it, though it's hard to say if that's due to the implementation's correctness or lack of awareness.

Either way, I think it's a good idea to enable it by default with the next major release.

@nfcampos
Copy link
Collaborator

I agree we should enable it by default in the next major. (do we keep the flag?)
(I've added a couple more test cases for this that I felt were missing in #681)

@laumair
Copy link

laumair commented Nov 13, 2016

👍 I've been using it extensively for over a month now I presume and it works great. I can maybe provide some examples as well as I've used it in many places.
However, even if used with mount it doesn't throw any warnings and just work. I think it should atleast display some sort of a warning when used with mount that it isn't necessary on mount or something. @ljharb @aweary

@nfcampos
Copy link
Collaborator

@laumair thanks for telling us your experience with it. glad to see it's been useful!
I agree that we should validate the options object and throw or warn when unknown keys are passed in.

@laumair
Copy link

laumair commented Nov 13, 2016

@nfcampos Thank YOU for this awesome library. Yeah validation would be much helpful if added.

@ljharb
Copy link
Member Author

ljharb commented Nov 13, 2016

@nfcampos i'd say we maybe rename the flag to deprecatedDisableLifecycleMethods or something, and keep it, but remove that in the next major.

@jwbay
Copy link
Contributor

jwbay commented Jan 24, 2017

I've also used this for months and seen no problems. I can work on a PR to make this the default.

@ljharb From your last comment, it sounds like you want to add a flag to suppress the lifecycle methods? A rename with the same functionality seems like it would be deprecatedEnableLifecycleMethods?

@ljharb
Copy link
Member Author

ljharb commented Jan 24, 2017

Yes, I think that would be the safe approach.

In fact, I'd prefer one semver-minor PR that adds "deprecatedDisableLifecycleMethods" (that right now is a no-op) and to publish that, and then later, a semver-major PR that changes the default behavior to enable the lifecycle methods and removes lifecycleExperimental, but respects the explicit disable flag as well.

@jwbay
Copy link
Contributor

jwbay commented Jan 28, 2017

Opened #789 with some questions.

@ljharb
Copy link
Member Author

ljharb commented Sep 26, 2017

Done in #1140.

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

No branches or pull requests

5 participants