-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
chore(playground): update dependencies and init #2849
base: v2
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for pinia-official ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pinia-playground ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
store.setImportMap({ | ||
imports: { | ||
...store.getImportMap().imports, | ||
...(import.meta.env.PROD | ||
? { | ||
pinia: '/pinia.esm-browser.js', | ||
'@vue/devtools-api': | ||
'https://cdn.jsdelivr.net/npm/@vue/[email protected]/lib/esm/index.js', | ||
'vue-demi': | ||
'https://cdn.jsdelivr.net/npm/[email protected]/lib/v3/index.mjs', | ||
} | ||
: { | ||
pinia: '/src/pinia-dev-proxy', | ||
vue: '/src/vue-dev-proxy', | ||
'vue/server-renderer': '/src/vue-server-renderer-dev-proxy', | ||
}), | ||
}, | ||
}) |
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 didn't totally understand what this block was doing, and removing didn't seem to impact the playground...?
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 think it's not needed anymore since it's done in the initialization
watchEffect(() => { | ||
const newHash = store | ||
.serialize() | ||
.replace(/^#/, useDevMode.value ? `#__DEV__` : `#`) |
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 removed this replace
logic; things still appear to be functional but please double-check
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.
Maybe it's now the __PROD__
that needs to be removed, maybe it's done by the serialize function now. For this kind of things I would look at the vuejs/core playground itself
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.
Thanks a lot!
- The
tsconfig
disappeared. We probably need to manually add the file (vuejs/repl@ca548b2) - The versions must be fully specified so I think we might need to fetch the latest version from npm when no hash is specified. It's fine to use Suspense for this
@@ -11,10 +11,11 @@ | |||
"devDependencies": { | |||
"@vitejs/plugin-vue": "^5.2.1", | |||
"execa": "^9.5.1", | |||
"typescript": "^5.7.2", |
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.
Do we need this? ts is installed at the root to make sure only one version is used (currently 5.6 because other things break)
vueVersion, | ||
productionMode, | ||
} = useVueImportMap({ | ||
runtimeDev: 'https://cdn.jsdelivr.net/npm/vue@3/dist/vue.runtime.esm-browser.js', |
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.
We actually really need to fetch the latest version from npm and use that specific tag. We cannot use @3
as a default because the idea is that a link should always work or fail the same way
vue: '/src/vue-dev-proxy', | ||
'vue/server-renderer': '/src/vue-server-renderer-dev-proxy', | ||
// Latest 5.x version | ||
'typescript': 'https://cdn.jsdelivr.net/npm/typescript@5/lib/typescript.min.js', |
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.
We shouldn't need typescript, the vue js playground doesn't have it
store.setImportMap({ | ||
imports: { | ||
...store.getImportMap().imports, | ||
...(import.meta.env.PROD | ||
? { | ||
pinia: '/pinia.esm-browser.js', | ||
'@vue/devtools-api': | ||
'https://cdn.jsdelivr.net/npm/@vue/[email protected]/lib/esm/index.js', | ||
'vue-demi': | ||
'https://cdn.jsdelivr.net/npm/[email protected]/lib/v3/index.mjs', | ||
} | ||
: { | ||
pinia: '/src/pinia-dev-proxy', | ||
vue: '/src/vue-dev-proxy', | ||
'vue/server-renderer': '/src/vue-server-renderer-dev-proxy', | ||
}), | ||
}, | ||
}) |
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 think it's not needed anymore since it's done in the initialization
watchEffect(() => { | ||
const newHash = store | ||
.serialize() | ||
.replace(/^#/, useDevMode.value ? `#__DEV__` : `#`) |
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.
Maybe it's now the __PROD__
that needs to be removed, maybe it's done by the serialize function now. For this kind of things I would look at the vuejs/core playground itself
watchEffect(() => history.replaceState({}, '', store.serialize())) | ||
|
||
// production mode is enabled | ||
productionMode.value = import.meta.env.PROD |
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.
We probably need to remove this. It's better to use the dev version by default (like in the vuejs playground)
Updates the Pinia Online Playground
@vue/repl
(I'm not super-familiar, so check my updates here)I was going to update the
tsconfig.json
to utilize"strict": true
but didn't dig in to where this is sourced.