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.

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

test: missing type module in ssr-vue package #8147

Merged
merged 3 commits into from May 13, 2022

Conversation

patak-dev
Copy link
Member

Description

I think this may be causing a race condition that results in this kind of glitches: https://github.com/vitejs/vite/runs/6406742290?check_suite_focus=true#step:11:85

The change in #7568 may make this glitch more common.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev
Copy link
Member Author

Interesting, so instead of fixing the issue adding "type": "module" make it reproducible. I may be missing something here 🤔

@patak-dev patak-dev mentioned this pull request May 13, 2022
4 tasks
@bluwy
Copy link
Member

bluwy commented May 13, 2022

The error doesn't seem to come from optimizeDeps but in node when it tries to load the Vue file, but it looks like with this change example-external-component is being externalized in SSR which should be not. Setting ssr.noExternal: ['example-external-component'] made it worked again.

Though I'm not sure why type: "module" made this more obvious, perhaps the file type mismatch caused Vite to no-external it previously.

@patak-dev
Copy link
Member Author

Looks like we were ignoring a warning before, this is the code path:

      const pkg = JSON.parse(pkgContent)
      if (pkg.type === 'module' || esmEntry.endsWith('.mjs')) {
        ssrExternals.add(id)
        continue
      }
      // check if the entry is cjs
      const content = fs.readFileSync(esmEntry, 'utf-8')
      if (CJS_CONTENT_RE.test(content)) {
        ssrExternals.add(id)
        continue
      }
      logger.warn(
        `${id} doesn't appear to be written in CJS, but also doesn't appear to be a valid ES module (i.e. it doesn't have "type": "module" or an .mjs extension for the entry point). Please contact the package author to fix.`
      )

When there isn't a type module, the package is wrong and Vite logs a warning and doesn't add it to SSR externals. Proper ESM packages are externalized now.

IIUC, @benmccann's PR to externalize by default would make this issue even more evident. @bluwy, users that want to use an ESM dependency with uncompiled .vue files should add it to ssr.noExternal, no? This isn't a bug in Vite, but an issue in the test setup. I'll add that to this PR.

@bluwy
Copy link
Member

bluwy commented May 13, 2022

Yup I think adding to ssr.noExternal is a fair change, and likely something end-users would do too. It's not common though since Vue libraries are often pre-compiled. In Svelte land, vite-plugin-svelte auto-adds Svelte libraries to ssr.noExternal by checking the pkg.svelte key (which isn't the most elegant solution).

@patak-dev patak-dev merged commit 4c325f6 into main May 13, 2022
@patak-dev patak-dev deleted the test/missing-type-module-in-ssr-vue branch May 13, 2022 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants