Migrate the usage of asyncData to fetch #10839

codyrancher · 2024-04-18T21:00:01Z

Summary

Migrate the usage of asyncData to fetch. I'll remove the nuxt implementation of asyncData in a separate pr (3cdf3ec).

This will give us one less thing that we have to consider while working on Vue3. After the migration we will then remove all of the initialization code.

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

The affected pages are, login, logout, setup, auth/config, and auth/roles.

All of these pages appear to be exercised through existing e2e tests.

Checklist

The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
The PR has a Milestone
The PR template has been filled out
The PR has been self reviewed
The PR has a reviewer assigned
The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

codyrancher · 2024-04-26T21:44:41Z

shell/pages/auth/login.vue

This page should be scrutinized the most during review. I wasn't able to do a 1 to 1 translation between asyncData and fetch primarly due to the previously used mounted() hook.

While figuring out what the issue was I broke up and shrunk the definition of asyncData and decided to leave it. If you'd like me to go back to a 1 to 1 definition let me know, I don't think it should be an issue.

codyrancher · 2024-04-26T21:47:11Z

shell/pages/auth/login.vue

+  data() {
+    const username = this.$cookies.get(USERNAME, { parseJSON: false }) || '';


Switching the access pattern of $cookies to be consistent with $route. It doesn't matter here but it matters when defining fetch as the code is treated completely differently by nuxt.

I would have expected all the data to come from one place only, which is meant to be the state management, while persistent data is to be collected together in one single object. We do not want to tackle this now though, so what you did for me is already totally fine and is a good start.

codyrancher · 2024-04-26T21:48:09Z

shell/pages/auth/login.vue

    this.username = this.firstLogin ? 'admin' : this.username;
    this.$nextTick(() => {
      this.focusSomething();
    });
  },

  methods: {
+    async loadInitialSettings() {


Pulled this out to reduce the amount I needed to understand while mentally parsing asyncData.

codyrancher · 2024-04-26T21:48:58Z

shell/pages/auth/login.vue

    this.username = this.firstLogin ? 'admin' : this.username;
    this.$nextTick(() => {
      this.focusSomething();
    });


This had to be moved from mounted. Previously asyncData would be executed and the state set before mounted was called. This caused big headaches while debugging e2e tests.

That's good you found out, thanks.
There's one aspect related to vendors and providers, which I am not sure we have covered in anyhow, but I guess it should be fine till other related variables are there as well.

codyrancher · 2024-04-26T21:50:22Z

shell/pages/auth/login.vue

+    if ( hasLocal ) {
+      // Local is special and handled here so that it can be toggled
+      removeObject(providers, 'local');
+    }


I love how we modify const lists. I really think immutable should take place of referentially const practically everywhere.

Our utilities are aimed at doing that, so it may be worth starting the conversation from that point since it's simpler.
For the record, I think me you and @rak-phillip are all in for that and we can even use ESLint for it, including Vue specific cases.

I'm all for it. Although I'm unsure if eslint will catch cases like this where we aren't explicitly reassigning the constant and mutating function parameters in removeObject..

codyrancher · 2024-04-26T21:50:55Z

shell/pages/auth/login.vue

-    const username = $cookies.get(USERNAME, { parseJSON: false }) || '';
-
-    return {
-      product: getProduct(),


This wasn't used anywhere in login

codyrancher · 2024-04-26T21:53:17Z

shell/pages/auth/setup.vue

+    this.$set(this, 'productName', productName);
+    this.$set(this, 'haveCurrent', !!current);
+    this.$set(this, 'username', me?.loginName || 'admin');
+    this.$set(this, 'isFirstLogin', isFirstLogin);
+    this.$set(this, 'mustChangePassword', mustChangePassword);
+    this.$set(this, 'current', current);
+    this.$set(this, 'v3User', v3User);
+    this.$set(this, 'serverUrl', serverUrl);
+    this.$set(this, 'mcmEnabled', mcmEnabled);
+    this.$set(this, 'telemetry', telemetry);
+    this.$set(this, 'principals', principals);


Some places we use $set and others we just use assignment. Is one better or easier for migration?

We are going to remove all the this.$set in the migration script with simple assignments, although it works differently between Vue2 and Vue3. The only concern is that my regex rule may not capture this case me?.loginName || 'admin' and will need to be modified by hand, but it's not a big deal.

Oh also that's a good point, no idea if we want specifically some cases to not be reactive, so a consistent assignment is preferred to avoid debugging later on.

$set should be an indicator that reactivity is desired if we are adding new properties to an already reactive object. I agree that assignment should be the default case, unless we're dealing with change detection caveats of Vue2

After the migration we will then remove all of the initialization code. This is motivated by the Vue3 migration.

codyrancher · 2024-04-26T22:11:48Z

shell/pages/auth/login.vue

-      showLocal:          !hasOthers || (route.query[LOCAL] === _FLAGGED),
-      firstLogin:         firstLoginSetting?.value === 'true',
-      singleProvider,
-      showLocaleSelector: !process.env.loginLocaleSelector || process.env.loginLocaleSelector === 'true'


Nothing about this code is async so I moved it to data.

cnotv

I cannot see any issue or valuable point to discuss that you did not already mention.

A topic for the future out of scope and which I already expressed is the presence of async patterns all the time from the state, which is really not necessary and caused clearly headaches also here.

LGTM

cnotv · 2024-04-29T10:14:09Z

shell/pages/auth/login.vue

+  data() {
+    const username = this.$cookies.get(USERNAME, { parseJSON: false }) || '';


I would have expected all the data to come from one place only, which is meant to be the state management, while persistent data is to be collected together in one single object. We do not want to tackle this now though, so what you did for me is already totally fine and is a good start.

cnotv · 2024-04-29T10:17:22Z

shell/pages/auth/login.vue

    this.username = this.firstLogin ? 'admin' : this.username;
    this.$nextTick(() => {
      this.focusSomething();
    });


That's good you found out, thanks.
There's one aspect related to vendors and providers, which I am not sure we have covered in anyhow, but I guess it should be fine till other related variables are there as well.

rak-phillip

Do you think that we'll be able to safely remove these lines about asyncData now that it's no longer used throughout Dashboard?

dashboard/shell/initialize/App.js

Lines 109 to 118 in 08f11a5

    
           if (page.$options.asyncData) { 
        
             p.push( 
        
               promisify(page.$options.asyncData, this.context) 
        
                 .then((newData) => { 
        
                   for (const key in newData) { 
        
                     Vue.set(page.$data, key, newData[key]); 
        
                   } 
        
                 }) 
        
             ); 
        
           }

dashboard/shell/initialize/client.js

Lines 382 to 393 in 08f11a5

    
           if (hasAsyncData) { 
        
             const promise = promisify(Component.options.asyncData, app.context); 
        
             promise.then((asyncDataResult) => { 
        
               applyAsyncData(Component, asyncDataResult); 
        
               if (this.$loading.increase) { 
        
                 this.$loading.increase(loadingIncrease); 
        
               } 
        
             }); 
        
             promises.push(promise); 
        
           }

edit: There's quite a few more references to asyncData in shell/initialize/client.js, but I don't think it makes sense to list them all here 🙂

cnotv · 2024-04-29T17:26:25Z

Do you think that we'll be able to safely remove these lines about asyncData now that it's no longer used throughout Dashboard?

As you mentioned we all agree there is a point for another issue and there are more cases, but since we do it I add a couple of comments.

There are more components using asyncData: https://github.com/search?type=code&auto_enroll=true&q=%22.asyncData%22+OR+%22+asyncData%22+repo%3Arancher%2Fdashboard

Also, let's not forget that something related to the routing can be handled with the related vue-router function. Albeit we may not want to fall in the same trap 😅

codyrancher · 2024-04-29T17:42:41Z

Do you think that we'll be able to safely remove these lines about asyncData now that it's no longer used throughout Dashboard?

dashboard/shell/initialize/App.js

Lines 109 to 118 in 08f11a5

if (page.$options.asyncData) {

p.push(

promisify(page.$options.asyncData, this.context)

.then((newData) => {

for (const key in newData) {

Vue.set(page.$data, key, newData[key]);

}

})

);

}

dashboard/shell/initialize/client.js

Lines 382 to 393 in 08f11a5

if (hasAsyncData) {

const promise = promisify(Component.options.asyncData, app.context);

promise.then((asyncDataResult) => {

applyAsyncData(Component, asyncDataResult);

if (this.$loading.increase) {

this.$loading.increase(loadingIncrease);

}

});

promises.push(promise);

}

edit: There's quite a few more references to asyncData in shell/initialize/client.js, but I don't think it makes sense to list them all here 🙂

Absolutely. I took care of that in this PR 3cdf3ec.

rak-phillip

LGTM - I gave the changes a quick smoke test and everything functions as it did before.

rak-phillip · 2024-04-29T17:09:41Z

shell/pages/auth/login.vue

+    if ( hasLocal ) {
+      // Local is special and handled here so that it can be toggled
+      removeObject(providers, 'local');
+    }


I'm all for it. Although I'm unsure if eslint will catch cases like this where we aren't explicitly reassigning the constant and mutating function parameters in removeObject..

rak-phillip · 2024-04-29T17:22:21Z

shell/pages/auth/setup.vue

+    this.$set(this, 'productName', productName);
+    this.$set(this, 'haveCurrent', !!current);
+    this.$set(this, 'username', me?.loginName || 'admin');
+    this.$set(this, 'isFirstLogin', isFirstLogin);
+    this.$set(this, 'mustChangePassword', mustChangePassword);
+    this.$set(this, 'current', current);
+    this.$set(this, 'v3User', v3User);
+    this.$set(this, 'serverUrl', serverUrl);
+    this.$set(this, 'mcmEnabled', mcmEnabled);
+    this.$set(this, 'telemetry', telemetry);
+    this.$set(this, 'principals', principals);


$set should be an indicator that reactivity is desired if we are adding new properties to an already reactive object. I agree that assignment should be the default case, unless we're dealing with change detection caveats of Vue2

github-actions bot assigned codyrancher Apr 18, 2024

codyrancher force-pushed the remove-async-data branch 4 times, most recently from 48eb5b8 to 68509e6 Compare April 25, 2024 00:33

codyrancher force-pushed the remove-async-data branch 3 times, most recently from 30de843 to 506adda Compare April 26, 2024 21:19

codyrancher added this to the v2.9.0 milestone Apr 26, 2024

codyrancher force-pushed the remove-async-data branch from 506adda to 0b51f10 Compare April 26, 2024 21:33

codyrancher commented Apr 26, 2024

View reviewed changes

codyrancher force-pushed the remove-async-data branch from 0b51f10 to ddd3bc3 Compare April 26, 2024 22:20

Migrate the usage of asyncData to fetch

7d282bf

After the migration we will then remove all of the initialization code. This is motivated by the Vue3 migration.

codyrancher force-pushed the remove-async-data branch from ddd3bc3 to 7d282bf Compare April 26, 2024 23:38

codyrancher commented Apr 27, 2024

View reviewed changes

codyrancher requested review from rak-phillip and cnotv April 27, 2024 00:35

codyrancher marked this pull request as ready for review April 27, 2024 00:35

cnotv approved these changes Apr 29, 2024

View reviewed changes

rak-phillip reviewed Apr 29, 2024

View reviewed changes

codyrancher merged commit e211993 into rancher:master Apr 29, 2024
27 checks passed

rak-phillip approved these changes Apr 29, 2024

View reviewed changes

richard-cox mentioned this pull request May 8, 2024

[v2.9] Missing Login button for external provider #10974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the usage of asyncData to fetch #10839

Migrate the usage of asyncData to fetch #10839

codyrancher commented Apr 18, 2024 •

edited

Loading

codyrancher Apr 26, 2024 •

edited

Loading

codyrancher Apr 26, 2024

cnotv Apr 29, 2024

codyrancher Apr 26, 2024 •

edited

Loading

codyrancher Apr 26, 2024

cnotv Apr 29, 2024

codyrancher Apr 26, 2024

cnotv Apr 29, 2024

rak-phillip Apr 29, 2024

codyrancher Apr 26, 2024

codyrancher Apr 26, 2024

cnotv Apr 29, 2024

cnotv Apr 29, 2024

rak-phillip Apr 29, 2024

codyrancher Apr 26, 2024

cnotv left a comment

cnotv Apr 29, 2024

cnotv Apr 29, 2024

rak-phillip left a comment •

edited

Loading

cnotv commented Apr 29, 2024

codyrancher commented Apr 29, 2024

rak-phillip left a comment

rak-phillip Apr 29, 2024

rak-phillip Apr 29, 2024

		data() {
		const username = this.$cookies.get(USERNAME, { parseJSON: false }) \|\| '';

	if (page.$options.asyncData) {
	p.push(
	promisify(page.$options.asyncData, this.context)
	.then((newData) => {
	for (const key in newData) {
	Vue.set(page.$data, key, newData[key]);
	}
	})
	);
	}

	if (hasAsyncData) {
	const promise = promisify(Component.options.asyncData, app.context);

	promise.then((asyncDataResult) => {
	applyAsyncData(Component, asyncDataResult);

	if (this.$loading.increase) {
	this.$loading.increase(loadingIncrease);
	}
	});
	promises.push(promise);
	}

Migrate the usage of asyncData to fetch #10839

Migrate the usage of asyncData to fetch #10839

Conversation

codyrancher commented Apr 18, 2024 • edited Loading

Summary

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

Checklist

codyrancher Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnotv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rak-phillip left a comment • edited Loading

Choose a reason for hiding this comment

cnotv commented Apr 29, 2024

codyrancher commented Apr 29, 2024

rak-phillip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher commented Apr 18, 2024 •

edited

Loading

codyrancher Apr 26, 2024 •

edited

Loading

codyrancher Apr 26, 2024 •

edited

Loading

rak-phillip left a comment •

edited

Loading