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

fix: ensure version query for direct node_modules imports #9848

Merged
merged 3 commits into from
Aug 27, 2022

Conversation

patak-dev
Copy link
Member

Fixes #7621
Fixes #2503 (with a different approach after reverting #2848)
Closes #9730

Potentially unblocks SvelteKit, as it will fix the issue described in #9828 if the direct node_modules import is in a JS source instead of a script tag. We aren't resolving the URL in script tags AFAICS, I'll check if this is something we should start doing. @Rich-Harris @benmccann would you confirm that this is something needed by SvelteKit, or it is just an artifact of how the reproduction was built (I'm assuming it is the later, and this PR may also then fix #9828)

Description

We mark non-optimized node_modules imports with a ?v={browserHash} query here

  if (skipOptimization) {
    // excluded from optimization
    // Inject a version query to npm deps so that the browser
    // can cache it without re-validation, but only do so for known js types.
    // otherwise we may introduce duplicated modules for externalized files
    // from pre-bundled deps.
    if (!isBuild) {
      const versionHash = depsOptimizer!.metadata.browserHash
      if (versionHash && isJsType) {
        resolved = injectQuery(resolved, `v=${versionHash}`)
      }
    }
  } 

This allows us to cache npm dependencies (even if they aren't optimized). See here

  const depsOptimizer = getDepsOptimizer(server.config, false) // non-ssr
  const type = isDirectCSSRequest(url) ? 'css' : 'js'
  const isDep =
    DEP_VERSION_RE.test(url) || depsOptimizer?.isOptimizedDepUrl(url)
  return send(req, res, result.code, type, {
    etag: result.etag,
    // allow browser to cache npm deps!
    cacheControl: isDep ? 'max-age=31536000,immutable' : 'no-cache',
    headers: server.config.server.headers,
    map: result.map
  })

We need the version query because once we re-optimize, we need to resend also the non-optimized node_modules files as they may have imported optimized modules so their imports needs to be rewritten to the new cached files.

Before this PR, we only added the version query to bare imports, and after #2848, also to relative imports of node_modules. After this PR, we are ensuring the version query also for all other ways to direct import a file in node_modules.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev
Copy link
Member Author

@Akryum I think this may also fix the issue behind #9661, but not the repro you provided. We need to check, as some of these paths depend on checking for /node_modules/ in the path. Maybe you could check this PR with histoire directly.

@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Aug 25, 2022
@patak-dev
Copy link
Member Author

vite-ecosystem-ci run against this PR. This is also testing the new 3.1-beta.0 PRs (object hooks and parse5 for HTML). There are two fails, but looks unrelated to the PR:

  • Storybook has an issue with object hooks (@IanVS if you have some time, it would be good to check this out)
  • vite-setup-catalogue (@sapphi-red, would you check if this is due to other changes in the 3.1 beta?)

@Rich-Harris
Copy link
Contributor

SvelteKit includes this in HTML:

<script type="module">
  import { start } from '/node_modules/@sveltejs/kit/src/runtime/client/start.js';
  // ...
</script>

Nothing else needs to import that file, but that file contains an import (which doesn't get ?v=xxx appended) of another module...

import { error } from '../../index/index.js';

...that is also exported from the package, hence the double imports:

import { error } from '@sveltejs/kit';

If this fix results in the imports inside start.js getting the ?v=xxx treatment, then I think it should work for us.

@IanVS
Copy link
Contributor

IanVS commented Aug 26, 2022

It looks like what's breaking is this line within a custom plugin we wrote to process mdx:

await reactRefresh?.transform!.call(this, modifiedCode, `${id}.jsx`, options);

Which we borrowed heavily from vite-plugin-mdx: https://github.com/brillout/vite-plugin-mdx/blob/ee4a732f19bd8ee072dc5257b96c3012f2cd6d75/src/index.ts#L58-L63

It looks like this might be due to rollup/rollup#4600. But it looks like that wasn't intended to be breaking. So, I'm not sure if there's something I need to change on my side (and alert @brillout to for his plugin as well), or if this is a bug in the rollup types.

@Rich-Harris
Copy link
Contributor

I've tested this branch with a reproduction of sveltejs/kit#5952, and it works perfectly! Thanks so much @patak-dev, this is excellent news

@sapphi-red
Copy link
Member

It seems it's a real regression of this PR. It does not happen with 3.1.0-beta.0.

When using this branch with file: (not link:), the following error happens.

Uncaught ReferenceError: __DEFINES__ is not defined
  1. pnpm create vite
  2. Select default for all options
  3. Add pnpm.overrides. Note that file: is used.
    "pnpm": {
      "overrides": {
        "vite": "file:/path/to/vite"
      }
    }
  4. pnpm i
  5. pnpm dev

@patak-dev
Copy link
Member Author

@sapphi-red I couldn't reproduce the issue with your instructions on MacOS, but I think vitejs/vite@f7785ab (#9848) could fix this. Would you check if it works for you?

@patak-dev
Copy link
Member Author

@brillout
Copy link
Contributor

LGTM.

(vite-plugin-mdx is deprecated so no probs on my end. Thanks for the ping.)

@sapphi-red
Copy link
Member

Ummm, I wonder why it didn't reproduce on macOS as ecosystem-ci runs on linux.
Putting that aside, it worked on Windows, too. 🎉

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

The changes LGTM! But would be great to have tests 😬

@patak-dev
Copy link
Member Author

@bluwy added tests in:

Also, good call, there was a bug with importing using a relative path.

@patak-dev patak-dev requested a review from bluwy August 26, 2022 16:23
// as if they would have been imported through a bare import
// Use the original id to do the check as the resolved id may be the real
// file path after symlinks resolution
const isNodeModule = !!normalizePath(id).match(nodeModulesInPathRE)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like most of the codebase simply checks by includes('node_modules'). I think we can probably do the same for consistency even if there might be false positives. But I don't feel strongly otherwise if we don't change this too.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a good mix now, other places tests for includes('/node_modules/'). Let's merge this as I think we should be more strict on this check, and we could update places only checking for 'node_modules' in future PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
6 participants