Removing the i18n middleware from the application #10923

codyrancher · 2024-05-01T03:11:16Z

Summary

Removing the i18n middleware from the application

Technical notes summary

The i18n middleware only initialized the i18n store. I moved the initialization elsewhere.

Areas or cases that should be tested

Translations on any page could be tested.

Screenshot/Video

i18n.mp4

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-05-01T22:35:43Z

shell/initialize/client.js

@@ -90,7 +90,7 @@ const errorHandler = Vue.config.errorHandler || console.error; // eslint-disable
 createApp(nuxt.publicRuntimeConfig).then(mountApp).catch(errorHandler); // eslint-disable-line no-undef

 function callMiddleware(Components, context) {
-  let midd = ['i18n'];


This was the only place I found it. If you can find it elsewhere let me know.

codyrancher · 2024-05-01T22:38:32Z

shell/initialize/App.js

+    // Ensure we initialize i18n for all pages
+    this.$store.dispatch('i18n/init');


If you have a better idea of where we should init this let me know.

I moved this from the store/index because the store/index didn't actually call it for every page. Notably, if you refreshed the login page after changing the language it would switch back to english.

Perhaps from within the install function of the i18n plugin itself? I think we have the store available to us at that point.

Honestly, I think that it fits here just as well. This is essentially the root of our app and dispatching some sort of init method makes sense at this point.

I like the idea. It doesn't look like we currently expose the store to the install function and useStore is only evailable in the setup function within that plugin. Is there a reason we shouldn't expose it?

We must install the plugin always after the store, since that's how it works, plus that's something which belongs to the plugin only and it's easier to find.
Since in the plugin install we have the options (as _options), should we not be able to dispatch it there?

codyrancher · 2024-05-01T22:39:02Z

shell/store/index.js

@@ -778,7 +778,6 @@ export const actions = {
    }

    res = await allHash(promises);
-    dispatch('i18n/init');


No point in having it here if we need it for every page and we're already making that call elsewhere.

codyrancher · 2024-05-02T17:57:21Z

shell/plugins/i18n.js

-console.warn('The implicit addition of i18n options has been deprecated in Rancher Shell and will be removed in a future version. Make sure to invoke `Vue.use(i18n)` to maintain compatibility.');
+// This is being done for backwards compatibility with our extensions that have written tests and didn't properly make use of Vue.use() when importing and mocking translations
+// Example failing test https://github.com/rancher/dashboard/actions/runs/8927503474/job/24521022320?pr=10923
+const isThisFileBeingExecutedInATest = process.env.JEST_WORKER_ID !== undefined || process.env.NODE_ENV === 'test';


I wasn't sure which would be true in our extensions so I used both.

Do we even want to use this pattern? It's a little odd but it provides the backwards compatibility while keeping our production environment in the ideal state.

AFAIK process.env.NODE_ENV === 'test' is the standard.
This is what we use in other configs:
https://github.com/rancher/dashboard/blob/master/shell/babel.config.js#L23-L28

Also, I've never seen the other one, but I dunno everything 😅

I did open an issue for Kubewarden to fix this¹. Maybe this is a scenario where we need to identify and update all extensions that are relying on this behavior.

Ideally, we would be able to remove this pattern after that's done.

Footnotes

https://github.com/rancher/kubewarden-ui/issues/679 ↩

Switched to process.env.NODE_ENV === 'test' and running the new tests.

And yeah, it's definitely temporary so I think it's fine to do for now.

codyrancher · 2024-05-02T23:10:53Z

Whenever you're ready, feel free to let me know if you'd like any additional changes or if you approve.

cnotv

LGTM

rak-phillip · 2024-05-02T23:35:36Z

jest.setup.js

@@ -14,7 +14,7 @@ global.TextEncoder = TextEncoder;
 global.TextDecoder = TextDecoder;

 Vue.config.productionTip = false;
-Vue.use(i18n);
+Vue.use(i18n, { store: { dispatch() {} } });


Should dispatch be mocked with jest.fn()?

I think it could actually just be entirely omitted now that I access it using the optional operator. If not, yeah we should mock it.

I'll address it in my next PR.

If we mock the dispatch, then we should mock everything else too.
In both cases we are not that strict though, as we do not test the i18n, we just want to not break it.

rancher-ui-project-bot bot assigned codyrancher May 1, 2024

codyrancher added this to the v2.9.0 milestone May 1, 2024

codyrancher commented May 1, 2024

View reviewed changes

codyrancher requested review from rak-phillip and cnotv May 1, 2024 22:39

codyrancher marked this pull request as ready for review May 2, 2024 01:14

codyrancher force-pushed the remove-i18n-middleware branch 4 times, most recently from c0a5986 to a47f883 Compare May 2, 2024 17:54

codyrancher commented May 2, 2024

View reviewed changes

Removing the i18n middleware from the application

5312b64

codyrancher force-pushed the remove-i18n-middleware branch from a47f883 to 5312b64 Compare May 2, 2024 22:10

cnotv approved these changes May 2, 2024

View reviewed changes

rak-phillip reviewed May 2, 2024

View reviewed changes

codyrancher merged commit be9cb77 into rancher:master May 3, 2024
26 checks passed

codyrancher mentioned this pull request May 6, 2024

Adding e2e tests for pages which use middleware to redirect #10945

Merged

7 tasks

cnotv mentioned this pull request Jul 17, 2024

Router architecture and authentication updates refactoring #11457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing the i18n middleware from the application #10923

Removing the i18n middleware from the application #10923

codyrancher commented May 1, 2024 •

edited

Loading

codyrancher May 1, 2024

codyrancher May 1, 2024

rak-phillip May 1, 2024

codyrancher May 2, 2024

cnotv May 2, 2024

codyrancher May 1, 2024

codyrancher May 2, 2024

cnotv May 2, 2024

rak-phillip May 2, 2024

codyrancher May 2, 2024

codyrancher commented May 2, 2024

cnotv left a comment

rak-phillip May 2, 2024

codyrancher May 3, 2024

cnotv May 3, 2024

		// Ensure we initialize i18n for all pages
		this.$store.dispatch('i18n/init');

Removing the i18n middleware from the application #10923

Removing the i18n middleware from the application #10923

Conversation

codyrancher commented May 1, 2024 • edited Loading

Summary

Technical notes summary

Areas or cases that should be tested

Screenshot/Video

Checklist

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

Footnotes

Choose a reason for hiding this comment

codyrancher commented May 2, 2024

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

Choose a reason for hiding this comment

codyrancher commented May 1, 2024 •

edited

Loading