-
Notifications
You must be signed in to change notification settings - Fork 266
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
Nuxt removal #7211
Nuxt removal #7211
Conversation
572a41d
to
1dd43af
Compare
I see the tests aren't passing so I'll take a look at that tomorrow. I think it's ready to have eyes on it though. |
1623a30
to
5bbe668
Compare
67678a4
to
af0ca33
Compare
The public entry was added. |
I debated whether I would respond since I'm not sure it will impact anything but I decided I wanted to just in case it might improve things in the future. I say this with respect, I know you put a good amount of effort and time into these reviews but I disagree with most of the above. It's so difficult for me to see a world in which the Typescript changes requested were a blocker.
If there are other changes you want done please let me know. I'm planning to have Neil take a look during my 1 on 1 today and if there's nothing more I'm going to update the routes and plugins in anticipation of a final look over. |
af0ca33
to
bd9a52e
Compare
Unless there's something wrong with autoload I don't think there's much left. |
bd9a52e
to
b7a64de
Compare
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
- rebased to resolve a merge conflict
- fixed websocket proxying for paths other than v1/v3/k8s (for example streaming container logs)
- created Remove
autoLoad
#8338
There was a failing test, if that passes though and once we branch for 2.7.2, lets get this in.
[Update]: Same gate failed. If @nwmac is happy merging this before it's fixed (and we branch) then it can merge
- This broke things like container log streaming - Fix is to dynamically find proxy to upgrade to - list of proxies to upgrade to is limited to those that initially have ws enabled - Also add typing
b7a64de
to
14ac4c4
Compare
The changes requested were resolved a while ago
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'm just doing an overview of the changes and I think there's a bunch of work which needs to be addressed and added in the TODO for the Vue3 migration.
It makes no sense to block for this, obviously, but we need to take notes.
#7653
ASAP I'll try to run and build it as part of the QA.
Some concerns go to the deployment process and the configuration with the paths, but it seems formally ok.
|
||
import '../assets/styles/app.scss' | ||
|
||
import _77180f1e from '../layouts/blank.vue' |
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 have some concerns about hardcoding hashed imports, as they may cause cache issues.
Let's make an issue to review this and apply proper naming maybe?
} | ||
|
||
const $loading = () => { | ||
const $nuxt = typeof window !== 'undefined' && window['$nuxt'] |
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.
This, as for the cache, should have an issue to remove Nuxt residual out of the code.
@@ -0,0 +1,143 @@ | |||
<template> |
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.
Can we document/comment concisely somewhere what is this code from the shell/nuxt
folder for, why do we need it and what are we planning to do?
Up to you either with a commit or an issue.
@@ -17,16 +17,16 @@ | |||
"rootDir": ".", |
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.
It does not make so much sense to have a default inside a folder and the one in the root importing it which imports another one 😅
We should make an issue where we review the TsConfigs we have around.
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.
Given #8376 is addressed soon, and we're not publishing shell, lets get this in.
General strategy:
Nuxt generates files for our application including things like route creation or plugin instantiation. I take all of the generated files and move them into shell/nuxt with minimal changes. I then migrate our nuxt.config.js to vue.config.js as close to 1 to 1 as I can. I then modify certain files if we no longer support certain aspects such as not supporting .ts/module imports in the vue.config.js or having to prepend @ to some of the style file imports.
Notes
fixes #5968