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.

Sign up for GitHub

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

Small dead code removal #10904

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Small dead code removal #10904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
241 changes: 0 additions & 241 deletions shell/initialize/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,10 @@ import fetch from 'unfetch';
import middleware from '../config/middleware.js';
import {
middlewareSeries,
sanitizeComponent,
resolveRouteComponents,
getMatchedComponents,
getMatchedComponentsInstances,
flatMapComponents,
setContext,
compile,
getQueryDiff,
globalHandleError,
isSamePath,
urlJoin
} from '../utils/nuxt.js';
import { createApp } from './index.js';
Expand All @@ -40,12 +34,9 @@ if (!global.fetch) {
}

// Global shared references
let _lastPaths = [];
let app;
let router;

const NUXT = {};

const $config = nuxt.publicRuntimeConfig || {}; // eslint-disable-line no-undef

if ($config._app) {
Expand Down Expand Up @@ -98,81 +89,6 @@ const errorHandler = Vue.config.errorHandler || console.error; // eslint-disable
// Create and mount App
createApp(nuxt.publicRuntimeConfig).then(mountApp).catch(errorHandler); // eslint-disable-line no-undef

async function loadAsyncComponents(to, from, next) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use asyncComponents in the app
https://v2.vuejs.org/v2/guide/components-dynamic-async#Async-Components

// Check if route changed (this._routeChanged), only if the page is not an error (for validate())
this._routeChanged = Boolean(app.nuxt.err) || from.name !== to.name;
this._paramChanged = !this._routeChanged && from.path !== to.path;
this._queryChanged = !this._paramChanged && from.fullPath !== to.fullPath;
this._diffQuery = (this._queryChanged ? getQueryDiff(to.query, from.query) : []);

if ((this._routeChanged || this._paramChanged) && this.$loading.start && !this.$loading.manual) {
this.$loading.start();
}

try {
if (this._queryChanged) {
const Components = await resolveRouteComponents(
to,
(Component, instance) => ({ Component, instance })
);
// Add a marker on each component that it needs to refresh or not
const startLoader = Components.some(({ Component, instance }) => {
const watchQuery = Component.options.watchQuery;

if (watchQuery === true) {
return true;
}
if (Array.isArray(watchQuery)) {
return watchQuery.some((key) => this._diffQuery[key]);
}
if (typeof watchQuery === 'function') {
return watchQuery.apply(instance, [to.query, from.query]);
}

return false;
});

if (startLoader && this.$loading.start && !this.$loading.manual) {
this.$loading.start();
}
}
// Call next()
next();
} catch (error) {
const err = error || {};
const statusCode = err.statusCode || err.status || (err.response && err.response.status) || 500;
const message = err.message || '';

// Handle chunk loading errors
// This may be due to a new deployment or a network problem
if (/^Loading( CSS)? chunk (\d)+ failed\./.test(message)) {
window.location.reload(true /* skip cache */);

return; // prevent error page blinking for user
}

this.error({ statusCode, message });
next();
}
}

// Get matched components
function resolveComponents(route) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing other code this was no longer used.

return flatMapComponents(route, async(Component, _, match, key, index) => {
// If component is not resolved yet, resolve it
if (typeof Component === 'function' && !Component.options) {
Component = await Component();
}

// Sanitize it and save it
Component._Ctor = sanitizeComponent(Component);

match.components[key] = Component;

return Component;
});
}

function callMiddleware(Components, context) {
let midd = ['i18n'];
let unknownMiddleware = false;
Expand Down Expand Up @@ -207,16 +123,6 @@ async function render(to, from, next) {
return next();
}

if (to === from) {
_lastPaths = [];
} else {
const fromMatches = [];

_lastPaths = getMatchedComponents(from, fromMatches).map((Component, i) => {
return compile(from.matched[fromMatches[i]].path)(from.params);
});
}
Comment on lines -210 to -218
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing other code _lastPaths was never read it was only written to.

The risk to removing code in a scenario like this is that a large portion of our code has side effects even in getters. I did look at getMatchedComponents and compile and neither appear to have side effects.


// nextCalled is true when redirected
let nextCalled = false;
const _next = (path) => {
Expand Down Expand Up @@ -269,13 +175,6 @@ async function render(to, from, next) {
return next();
}

// Update ._data and other properties if hot reloaded
Components.forEach((Component) => {
if (Component._Ctor && Component._Ctor.options) {
Component.options.fetch = Component._Ctor.options.fetch;
}
});
Comment on lines -273 to -277
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rak-phillip it looks like you were right and we can remove this.

In all the cases I saw when invoking this the method of Component.options.fetch was equal to Component._Ctor.options.fetch so this assignment didn't do anything.

I also verified that HMR was working when modifying code in Project Namespaces.


try {
// Call middleware
await callMiddleware.call(this, Components, app.context);
Expand Down Expand Up @@ -327,70 +226,6 @@ async function render(to, from, next) {
return next();
}

let instances;

// Call fetch hooks on components matched by the route.
await Promise.all(Components.map((Component, i) => {
Copy link
Contributor Author

@codyrancher codyrancher Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ultimately this could be removed because we'd exit early from this before doing any mutations. Our fetch method is still called which is obvious by our e2e tests still passing.

// Check if only children route changed
Component._path = compile(to.matched[matches[i]].path)(to.params);
Component._dataRefresh = false;
const childPathChanged = Component._path !== _lastPaths[i];

// Refresh component (call fetch) when:
// Route path changed part includes current component
// Or route param changed part includes current component and watchParam is not `false`
// Or route query is changed and watchQuery returns `true`
if (this._routeChanged && childPathChanged) {
Component._dataRefresh = true;
} else if (this._paramChanged && childPathChanged) {
const watchParam = Component.options.watchParam;

Component._dataRefresh = watchParam !== false;
} else if (this._queryChanged) {
const watchQuery = Component.options.watchQuery;

if (watchQuery === true) {
Component._dataRefresh = true;
} else if (Array.isArray(watchQuery)) {
Component._dataRefresh = watchQuery.some((key) => this._diffQuery[key]);
} else if (typeof watchQuery === 'function') {
if (!instances) {
instances = getMatchedComponentsInstances(to);
}
Component._dataRefresh = watchQuery.apply(instances[i], [to.query, from.query]);
}
}
if (!this._hadError && this._isMounted && !Component._dataRefresh) {
return;
}

const promises = [];

const hasFetch = Boolean(Component.options.fetch) && Component.options.fetch.length;

const loadingIncrease = hasFetch ? 30 : 45;

// Check disabled page loading
this.$loading.manual = Component.options.loading === false;

// Call fetch(context)
if (hasFetch) {
let p = Component.options.fetch(app.context);

if (!p || (!(p instanceof Promise) && (typeof p.then !== 'function'))) {
p = Promise.resolve(p);
}
p.then((fetchResult) => {
if (this.$loading.increase) {
this.$loading.increase(loadingIncrease);
}
});
promises.push(p);
}

return Promise.all(promises);
}));

// If not redirected
if (!nextCalled) {
if (this.$loading.finish && !this.$loading.manual) {
Expand All @@ -402,8 +237,6 @@ async function render(to, from, next) {
} catch (err) {
const error = err || {};

_lastPaths = [];

globalHandleError(error);

this.error(error);
Expand Down Expand Up @@ -432,52 +265,6 @@ function checkForErrors(app) {
}
}

// When navigating on a different route but the same component is used, Vue.js
// Will not update the instance data, so we have to update $data ourselves
function fixPrepatch(to, ___) {
Copy link
Contributor Author

@codyrancher codyrancher Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We would early exit because _dataRefresh was never true (Because we'd always exit early from the // Call fetch hooks on components matched by the route. section).

This was changing behavior from standard vue and I verified that data was still loading by going to two child routes which shared the same components. More specifically, two list pages which hadn't overridden the default (a few random ones under more resources).

if (this._routeChanged === false && this._paramChanged === false && this._queryChanged === false) {
return;
}

const instances = getMatchedComponentsInstances(to);
const Components = getMatchedComponents(to);

Vue.nextTick(() => {
instances.forEach((instance, i) => {
if (!instance || instance._isDestroyed) {
return;
}

if (
instance.constructor._dataRefresh &&
Components[i] === instance.constructor &&
instance.$vnode.data.keepAlive !== true &&
typeof instance.constructor.options.data === 'function'
) {
const newData = instance.constructor.options.data.call(instance);

for (const key in newData) {
Vue.set(instance.$data, key, newData[key]);
}
}
});

checkForErrors(this);
});
}

function nuxtReady(_app) {
window.onNuxtReadyCbs.forEach((cb) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onNuxtReadyCbs wenre't being added because there weren't any invocations of onNuxtReady. Without onNuxtReadyCbs this invocation is useless.

if (typeof cb === 'function') {
cb(_app);
}
});
// Special JSDOM
if (typeof window._onNuxtLoaded === 'function') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No usages of _onNuxtLoaded found.

window._onNuxtLoaded(_app);
}
}

async function mountApp(__app) {
// Set global variables
app = __app.app;
Expand All @@ -492,31 +279,12 @@ async function mountApp(__app) {

// Add afterEach router hooks
router.afterEach(normalizeComponents);

router.afterEach(fixPrepatch.bind(_app));

// Listen for first Vue update
Vue.nextTick(() => {
// Call window.{{globals.readyCallback}} callbacks
nuxtReady(_app);
});
Comment on lines -499 to -502
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explained why nuxtReady wasn't doing anything in a comment above.

};

// Resolve route components
const Components = await Promise.all(resolveComponents(app.context.route));

if (Components.length) {
_lastPaths = router.currentRoute.matched.map((route) => compile(route.path)(router.currentRoute.params));
}
Comment on lines -506 to -510
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_lastPaths was only written to after other changes.


// Initialize error handler
_app.$loading = {}; // To avoid error while _app.$nuxt does not exist
if (NUXT.error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No usages of NUXT.error. It's only scoped to this file.

_app.error(NUXT.error);
}

// Add beforeEach router hooks
router.beforeEach(loadAsyncComponents.bind(_app));
router.beforeEach(render.bind(_app));
router.beforeEach((from, to, next) => {
if (from?.name !== to?.name) {
Expand All @@ -526,19 +294,10 @@ async function mountApp(__app) {
next();
});

// Fix in static: remove trailing slash to force hydration
// Full static, if server-rendered: hydrate, to allow custom redirect to generated page

// Fix in static: remove trailing slash to force hydration
if (NUXT.serverRendered && isSamePath(NUXT.routePath, _app.context.route.path)) {
return mount();
}
Comment on lines -533 to -535
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No SSR.


// First render on client-side
const clientFirstMount = () => {
normalizeComponents(router.currentRoute, router.currentRoute);
checkForErrors(_app);
// Don't call fixPrepatch.call(_app, router.currentRoute, router.currentRoute) since it's first render
mount();
};

Expand Down
Loading
Loading