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

Deprecate bound action creators for "actions" #3491

Merged
merged 6 commits into from
Jan 13, 2018

Conversation

calcsam
Copy link
Contributor

@calcsam calcsam commented Jan 12, 2018

Fixes #1909

@ghost ghost assigned calcsam Jan 12, 2018
@ghost ghost added the review label Jan 12, 2018
@@ -350,6 +352,19 @@ module.exports = async (program: any) => {
console.log()
}

function printDeprecationWarnings() {
const files = glob
.sync("{,!(node_modules|public)/**/}*.js")
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be pretty expensive. I was thinking that instead, we could wrap the functions we pass to boundActionCreators so that any time those functions are called, we write to console (only once per session) a message saying that they should switch to actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is nice you say which files are using it — we do know which plugins we're calling so we could probably capture that data as well to improve the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not too expensive -- just searches through the gatsby repo for any source code javascript files. I timed it on the gatsbyjs.org website and it takes 0.044 seconds the first time, 0.02 seconds the second time, 0.013 seconds the third time (probably due to bits of memory getting pulled into caches).

Also this only gets called once per gatsby develop, if we are worried about performance in the future, we can make it async so it's not blocking. In general too this seems like a pretty nice structure for other helpful messages we may want to add in the future. "We see you're using plugin X, this has been deprecated in favor of plugin Y"

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, I thought this was searching all of node_modules. This is only alerting on site specific code not 3rd party plugins. Makes sense.

@KyleAMathews
Copy link
Contributor

This PR should also replace all instances of boundActionCreators in plugins/example sites with actions

@KyleAMathews KyleAMathews changed the title Deprecate bound action creators Deprecate bound action creators for "actions" Jan 12, 2018
@calcsam
Copy link
Contributor Author

calcsam commented Jan 12, 2018

👍 done

@KyleAMathews KyleAMathews merged commit 04931e6 into v2 Jan 13, 2018
@KyleAMathews KyleAMathews deleted the deprecate-bound-action-creators branch January 13, 2018 00:36
@ghost ghost removed the review label Jan 13, 2018
@KyleAMathews
Copy link
Contributor

Sweet!

@TuckerWhitehouse TuckerWhitehouse mentioned this pull request Feb 16, 2018
13 tasks
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
* deprecate bound action creators for v2, remove from website, add error message

* remove find-in-files

* modify plugins to replace boundActionCreators with actions

* modify examples to replace boundActionCreators with actions

* remove merge artifact
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