-
Notifications
You must be signed in to change notification settings - Fork 314
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
Migrate all React functionality to use @wordpress/element with dependency extraction #2756
Merged
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
063b049
Migrate React dependencies to @wordpress/element.
JakePT 5afff77
Fix dependency name.
JakePT fca51b4
Replace third party dependency with core component.
JakePT eeb2743
Remove autosuggest polyfill dependencies.
JakePT 2233d83
Merge branch 'develop' into feature/2613
JakePT d704eb7
Merge branch 'develop' into feature/2613
felipeelia c509259
Update synonyms test.
JakePT 561bd22
Merge branch 'feature/2613' of github.com:10up/ElasticPress into feat…
JakePT a47f6cb
Merge branch 'develop' into feature/2613
JakePT 94f2b93
Revert to regular fetch for Autosuggest.
JakePT 239c83c
Merge branch 'develop' into feature/2613
felipeelia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,11 @@ | ||
import ReactDOM from 'react-dom'; | ||
/** | ||
* WordPress dependencies. | ||
*/ | ||
import { render } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal dependencies. | ||
*/ | ||
import { Pointers } from './pointers'; | ||
|
||
ReactDOM.render(<Pointers />, document.getElementById('ordering-app')); | ||
render(<Pointers />, document.getElementById('ordering-app')); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI:
@wordpress/api-fetch
will force loading lodash on the front-end which is a huge dep. I think you should stick tofetch
on the front-end.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also recommend looking into this: https://github.com/10up/10up-toolkit/blob/develop/packages/toolkit/README.md#-react--wordpress
There are a few things you can prob dequeue on the front-end (assuming IE 11 is not supported anymore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicholasio Thanks for the tip re: fetch.
Regarding dequeuing the polyfill, we aren't supporting IE11 ourselves, but it looks like the suggestions would remove the polyfill as a dependency of
wp-element
for all themes and plugins that use it, not just ElasticPress, and I don't think that's something we'd want to do as a publicly distributed plugin. Thoughts @felipeelia?We could remove
wp-polyfill
as a dependency of our scripts in theget_asset_info()
function, but if it's a dependecy ofwp-element
anyway there's probably not much point.Open to any other ideas though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the change introduced in this PR start to enqueue lodash and wp-polyfill or is that something that is already happening?
Curious to hear from @nicholasio about that anyway. It sounds like if someone out there is relying on those packages and we simply dequeue them in the name of performance, things will break. In that case, we won't be touching it in this release.
In general, that sounds more like an optimization that site owners should apply rather than something we could make in the plugin -- although the performance gain would be generally more significant for most of the sites, we obviously will only hear from the sites it actually broke what was already working before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are both right, doing this optimization on the plugin wouldn't make sense! I'd however try to stick with standard apis on the front-end. Most of the
@wordpress/packages
are not optimized for usage outise of the editor. Most of them will load lodash (and some will load moment.js) both well know to be huge packages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least I'd move away from
@wordpress/date
as@wordpress/element
can be optimized by site owners that do not support IE 11 (and I know wordpress core is trying to optimize the polyfills they add as well)Moving away from
@wordpress/element
means shipping react which could cause multiple react to be enqueed on the front-end if other plugins are also relying on react or wordpress/element.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicholasio
@wordpress/date
does things that we need, namely take raw date data and display it in the site's timezone and language in JS. In the future we'd probably also want to let site owners or devs use their own date format as well. Would you recommend a different approach?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are smaller libraries tath can do what you need. Look at
daysjs
https://day.js.org/docs/en/plugin/utcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicholasio Ok cool we'll check that out. @felipeelia In the meantime, this PR isn't introducing that package so it probably doesn't need to hold this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've filed an issue to look into those packages #2768