Import & Run #86

dainemawer · 2022-02-28T12:12:57Z

Description of the Change

Fixes #80

Alternate Designs

Possible Drawbacks

Verification Process

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests passed.

Changelog Entry

Credits

Props @

dainemawer · 2022-02-28T12:15:30Z

@rdimascio - I noticed a strange bug here. The navigation component should not run unless the selectors array matches, but it seems to run regardless. Do you mind taking a look?

rdimascio · 2022-03-02T09:34:44Z

Thanks for putting this together @dainemawer! I fixed the bug you mentioned, cleaned up the architecture a bit, added error handling and some documentation. This is looking good to me.

cc @joesnellpdx @devinle @nicholasio @fabiankaegy

devinle · 2022-03-02T16:22:06Z

@rdimascio @dainemawer this is looking good to me. Nice and clean - easy to follow. I also like the ability to pass a custom callback. Well done!

joesnellpdx · 2022-03-02T22:46:32Z

Per discussion with @rdimascio ...

Is this something we want to include in 90% of our projects, or are there particular use cases that would trigger the usage of import-and-run?

If it is not something we want to have as default, do we consider one of the following?:

Add as an enhancements section in the README - with guidance / examples
Add a new enhancements doc - link to it in the README - and include guidance / examples for this
Add an examples directory in the project that is not included with references in the README file / comments
Add to existing architecture, but comment out with comment guidance when/ how to activate (and add to README)

Either way, there needs to be more clear guidance and documentation for engineers if this is implemented.

@devinle @dainemawer

nicholasio

What are we thinking in terms of recommendations for this?

Are we going to essentially code split every single front-end component?

I do not think code-splitting every single front-end component is worth it. There's an overhead of making an additional call to load the JS. If we ever ship something like this I feel we need more guidance around when to use it. While I do not have any benchmark to back me up here I do not think code-splitting very small components would actually give any meaningful improvement, while potentially making things a little worse. We should also be mindful of CLS, if every single front-end component is lazy-loaded, this might mean we're delaying fully rendering stuff on the front end which could cause content shifting

nicholasio · 2022-03-04T18:15:13Z

themes/10up-theme/assets/js/frontend/components/navigation.js

@@ -0,0 +1,5 @@
+export default class Navigation {


Does this actually need to be a class?

It does not, but I think the large majority of our projects are still using class based components. I added a comment for sites with functional components.

@nicholasio this was just added for example sake - as long as this component is the default export we should be okay. Happy to update it to a const Navigation = () => {}

rdimascio · 2022-03-04T19:30:42Z

@nicholasio I would think that script parse and execution time on page load would result in a worse user experience and CWV scores than network requests. I don't have any benchmarks for this either (and @dainemawer can probably provide more insight here) but I believe the implementation for this approach (when and even how to use it) will larger depend on project architecture. I'm in agreement that there needs to be more clear guidance and documentation on this.

nicholasio · 2022-03-04T20:12:39Z

xecution time on page load would result in a worse user experience and CWV scores than network requests. I don't have any benchmarks for this either (and @dainemawer can probably provide more insight here) but I believe the implementation for this approach (when and even how to use it) will larger depend on project architecture. I'm in agreement that there needs to be more clear guidance and documentation on this.

You're right but it depends on what you're code splitting. Every optimization comes with an overhead cost. Finding the balance is what we're missing here. We're all on the same page we need more documentation and guidance here 👍

dainemawer · 2022-03-06T13:58:48Z

What are we thinking in terms of recommendations for this?

Are we going to essentially code split every single front-end component?

I do not think code-splitting every single front-end component is worth it. There's an overhead of making an additional call to load the JS. If we ever ship something like this I feel we need more guidance around when to use it. While I do not have any benchmark to back me up here I do not think code-splitting very small components would actually give any meaningful improvement, while potentially making things a little worse. We should also be mindful of CLS, if every single front-end component is lazy-loaded, this might mean we're delaying fully rendering stuff on the front end which could cause content shifting

@nicholasio - yeah I agree here, I don't think the intention is to code-split every component. What this PR lacks, as @joesnellpdx mentioned above, is documentation is going forward. The idea here should be to code-split out:

Any resource heavy functions / components - potentially components requiring third-party libs like lodash, moment etc.
Any scripts that do not need to be loaded in the bundle on initial load - chat libs, cookie banners etc.

I will add some guidance here. I'll also clean up this entry file so that its more instructional.

dainemawer · 2022-03-06T14:03:07Z

In general, benchmarks are a bit all over the place. They are specific to the nature of the site as well. The overall strategy here is to offload any components and scripts that are not absolutely needed for a good user experience on initial load - or above the fold.

An example of this would be something along the lines of:

We may write a component in JS for the Navigation. That JS should not be lazy-loaded. It's imperative for good UX on initial load.
A cookie banner though, whose presence is not in the flow of the document, and does not contribute to important user experience.

Doing this above, and identifying elements with the above nature, means we can keep the byte size of our bundle low.

tylercherpak

@dainemawer I love this and think it has a ton of potential to be part of how we think about components and if they are needed at time of initial paint.

One question I want to raise for everyone, do we think we should have an example in which making the component dynamic is more necessary. Such as a slider that needs to include additional libraries that we don't want to load at render or if that component isn't present on the page.

Let me know what you think! Thanks again for this!! 😄

tylercherpak · 2022-08-17T13:14:21Z

composer.json

+  },
+  "config": {
+    "allow-plugins": {
+      "dealerdirect/phpcodesniffer-composer-installer": true


@dainemawer is this directly related to the dynamic imports?

tylercherpak · 2022-08-17T13:25:27Z

themes/10up-theme/assets/js/frontend/components/index.js

+export const initComponents = () => {
+	Object.entries(COMPONENTS).forEach(([component, args]) => {
+		importAndRun(
+			`frontend/components/${component}`,


are we importing and running the un-minified js source? Should webpack be creating a separate bundle for this component?

dainemawer added 2 commits February 28, 2022 13:45

Base functionality for import and run

d55a028

Added test case, added context for default function export

0be66aa

dainemawer requested a review from rdimascio February 28, 2022 12:12

dainemawer changed the title ~~Fixes #80 - Import & Run~~ Import & Run Feb 28, 2022

rdimascio added 2 commits March 2, 2022 01:23

Fix and clean up dynamic module loading

a34a0c8

Update example navigation component selectors

a2dd1f5

rdimascio approved these changes Mar 2, 2022

View reviewed changes

rdimascio requested a review from joesnellpdx March 2, 2022 09:35

nicholasio reviewed Mar 4, 2022

View reviewed changes

tylercherpak reviewed Aug 17, 2022

View reviewed changes

fabiankaegy marked this pull request as draft February 20, 2024 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import & Run #86

Import & Run #86

dainemawer commented Feb 28, 2022 •

edited

Loading

dainemawer commented Feb 28, 2022

rdimascio commented Mar 2, 2022

devinle commented Mar 2, 2022

joesnellpdx commented Mar 2, 2022

nicholasio left a comment •

edited

Loading

nicholasio Mar 4, 2022

rdimascio Mar 4, 2022

dainemawer Mar 6, 2022

rdimascio commented Mar 4, 2022

nicholasio commented Mar 4, 2022

dainemawer commented Mar 6, 2022

dainemawer commented Mar 6, 2022

tylercherpak left a comment

tylercherpak Aug 17, 2022

tylercherpak Aug 17, 2022

Import & Run #86

Are you sure you want to change the base?

Import & Run #86

Conversation

dainemawer commented Feb 28, 2022 • edited Loading

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Checklist:

Changelog Entry

Credits

dainemawer commented Feb 28, 2022

rdimascio commented Mar 2, 2022

devinle commented Mar 2, 2022

joesnellpdx commented Mar 2, 2022

nicholasio left a comment • edited Loading

Choose a reason for hiding this comment

nicholasio Mar 4, 2022

Choose a reason for hiding this comment

rdimascio Mar 4, 2022

Choose a reason for hiding this comment

dainemawer Mar 6, 2022

Choose a reason for hiding this comment

rdimascio commented Mar 4, 2022

nicholasio commented Mar 4, 2022

dainemawer commented Mar 6, 2022

dainemawer commented Mar 6, 2022

tylercherpak left a comment

Choose a reason for hiding this comment

tylercherpak Aug 17, 2022

Choose a reason for hiding this comment

tylercherpak Aug 17, 2022

Choose a reason for hiding this comment

dainemawer commented Feb 28, 2022 •

edited

Loading

nicholasio left a comment •

edited

Loading