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

fix: check exactly in proxyESM #5512

Merged
merged 5 commits into from
Nov 11, 2021
Merged

fix: check exactly in proxyESM #5512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a2b3b62
fix: check strict in proxyESM
Nov 2, 2021
48e8b77
test: add test code for proxyESM import vuex
Nov 2, 2021
7180327
test: update
Nov 2, 2021
943d0e3
fix: merge conflict
Nov 8, 2021
916a67c
fix: merge conflict
Nov 8, 2021
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
10 changes: 10 additions & 0 deletions packages/playground/ssr-vue/__tests__/ssr-vue.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ import fetch from 'node-fetch'

const url = `http://localhost:${port}`

test('vuex can be import succeed by named import', async () => {
await page.goto(url + '/store')
expect(await page.textContent('h1')).toMatch('bar')

// raw http request
const storeHtml = await (await fetch(url + '/store')).text()
expect(storeHtml).toMatch('bar')
})


test('/about', async () => {
await page.goto(url + '/about')
expect(await page.textContent('h1')).toMatch('About')
Expand Down
3 changes: 2 additions & 1 deletion packages/playground/ssr-vue/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
"dependencies": {
"example-external-component": "file:example-external-component",
"vue": "^3.2.21",
"vue-router": "^4.0.0"
"vue-router": "^4.0.0",
"vuex": "^4.0.2"
},
"devDependencies": {
"@vitejs/plugin-vue": "workspace:*",
Expand Down
24 changes: 24 additions & 0 deletions packages/playground/ssr-vue/src/pages/Store.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<template>
<h1>{{ foo }}</h1>
</template>

<script>
import { createStore } from 'vuex'

export default {
async setup() {
const store = createStore({
state: {
foo: 'bar'
}
})
return store.state
}
}
</script>

<style scoped>
h1 {
color: red;
}
</style>
2 changes: 1 addition & 1 deletion packages/vite/src/node/ssr/ssrModuleLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ function proxyESM(id: string, mod: any) {
return new Proxy(mod, {
get(mod, prop) {
if (prop === 'default') return defaultExport
return mod[prop]
return mod[prop] ?? defaultExport?.[prop]
Copy link
Member

Choose a reason for hiding this comment

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

This approach is fragile. If a named export equals undefined or null, the default export is assumed to be an object, but we can't guarantee that.

#5197 was using this logic in proxyESM...

if (prop in mod) {
  return mod[prop]
}
// commonjs interop: module whose exports are not statically analyzable
if (isObject(mod.default) && prop in mod.default) {
  return mod.default[prop]
}
// throw an error like ESM import does
throw new SyntaxError(
  `The requested module '${id}' does not provide an export named '${prop.toString()}'`
)

...until @matthewp reverted it without explanation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for digging into this, @natemoo-re @matthewp could you check why this was removed? Maybe as part of the trials to make CI pass? Let's add this back before releasing 2.7

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this was removed, but this approach LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

The throw was a problem there. Since mod is an object, any code that imports the namespace can inspect it for keys just like you can do with any object. The throw assumed that the get always comes from a static import statement but that's not necessarily the case:

// This is valid code
import * as mod from './something.js';

if(mod.someKeyThatDoesntExist) {
  // Oops this threw
}

Also the second condition looks wrong to me too. Given this example:

mod.js

export default { color: 'green' };

main.js

import * as mod from './mod.js';

console.log(mod.color);

Should log undefined but will log 'green' instead. If commonjs interop is needed it needs to verify that the module is commonjs first.

}
})
}
Expand Down
2 changes: 2 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.