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(resolve): normalize optimized resolved path #4813

Merged
merged 1 commit into from Sep 1, 2021

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 1, 2021

Description

Fix path normalization when trying to resolve an optimized path. This is because the id was normalized here and then passed to here, and when comparing it here, resolvedSrc also needs to be normalized.

Additional context

  1. Follow-up on feat(optimizer): nested optimization #4634, I'm not sure why the tests passed for windows. I tried to make a failing test for this to no avail.
  2. Blocking wip: enable nested optimization  sveltejs/vite-plugin-svelte#157

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Sep 1, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Just to be curious, why are all tests passing right now? Do we not have a test case for this?

@bluwy
Copy link
Member Author

bluwy commented Sep 1, 2021

We did have a test case for this that was added in #4634, under packages/playground/optimize-deps where the nested-exclude package imports the nested-include package (cjs only). In Windows, it should fail since tryOptimizedResolve wouldn't have resolved the nested-include package, but it passed so I believe there could be something up with the local test setup, because I'm able to reproduce the issue externally on Windows

@patak-dev patak-dev merged commit fa6475f into vitejs:main Sep 1, 2021
@dominikg
Copy link
Contributor

dominikg commented Sep 1, 2021

a little late but i can confirm that linking to a local vite build with this change makes vite-plugin-svelte's test-suite pass

c:\Users\dominikg\develop\sveltejs\vite-plugin-svelte>pnpm test:ci:serve

> vite-plugin-svelte-monorepo@ test:ci:serve c:\Users\dominikg\develop\sveltejs\vite-plugin-svelte
> cross-env VITE_PRESERVE_BUILD_ARTIFACTS=1 jest --verbose --no-cache --runInBand --force-exit --ci --json --outputFile="temp/serve/jest-results.json"

Determining test suites to run...
preparing non ci env...
syncing node_modules
syncing node_modules done
building packages

> vite-plugin-svelte-monorepo@ build:ci c:\Users\dominikg\develop\sveltejs\vite-plugin-svelte
> pnpm --dir packages/vite-plugin-svelte build:ci


> @sveltejs/vite-plugin-svelte@1.0.0-next.19 build:ci C:\Users\dominikg\develop\sveltejs\vite-plugin-svelte\packages\vite-plugin-svelte
> rimraf dist && tsup-node src/index.ts --format esm,cjs --no-splitting

 CLI  Building entry: src/index.ts
 CLI  Using tsconfig: C:\Users\dominikg\develop\sveltejs\vite-plugin-svelte\packages\vite-plugin-svelte\tsconfig.json
 CLI  tsup v4.14.0
 CLI  Target: node12
 ESM  Build start
 CJS  Build start
 CJS  Build success in 174ms
 ESM  Build success in 294ms
building packages done
preparations done
 PASS  packages/e2e-tests/kit-node/__tests__/kit.spec.ts (13.415 s)
  kit-node
    index route
      √ should hydrate (447 ms)
      √ should have correct styles applied (15 ms)
      √ should increase count on click (82 ms)
      √ should not have failed requests
      √ should load dynamic import in onMount
      hmr
        √ should render additional html (719 ms)
        √ should render additional child components (453 ms)
        √ should apply changed styles (254 ms)
        √ should serve changes even after page reload (1014 ms)
        child component update
          √ should preserve dom order (211 ms)
          √ should render additional html (301 ms)
          √ should apply changed styles (228 ms)
          √ should apply changed initial state (245 ms)
        config file update
          √ should show an overlay (106 ms)

 PASS  packages/e2e-tests/hmr/__tests__/hmr.spec.ts (14.221 s)
  √ should render App (40 ms)
  √ should render static import (3 ms)
  √ should render dependency import (16 ms)
  √ should render dynamic import (251 ms)
  √ should not have failed requests
  hmr
    √ should have expected initial state (47 ms)
    √ should have working increment button (172 ms)
    √ should apply css changes in HmrTest.svelte (174 ms)
    √ should apply js change in HmrTest.svelte  (234 ms)
    √ should keep state of external store intact on change of HmrTest.svelte (252 ms)
    √ should preserve state of external store used by HmrTest.svelte when editing App.svelte (241 ms)
    √ should preserve state of store when editing hmr-stores.js (202 ms)
 PASS  packages/e2e-tests/vite-ssr/__tests__/vite-ssr.spec.ts (5.599 s)
  √ / (35 ms)
  √ css (37 ms)
  √ asset (15 ms)
  hmr
    √ should render additional html (332 ms)
    √ should apply style update (166 ms)
    √ should not preserve state of updated props (373 ms)

 PASS  packages/e2e-tests/autoprefixer-browerslist/__tests__/autoprefixer-browerslist.spec.ts
  √ should prefix position: sticky for code in source tree (34 ms)

 PASS  packages/e2e-tests/package-json-svelte-field/__tests__/package-json-svelte-field.spec.ts
  √ should render component imported via svelte field in package.json (65 ms)
  √ should optimize nested deps of excluded svelte deps

 PASS  packages/vite-plugin-svelte/src/utils/__tests__/dependencies.spec.ts
  dependencies
    findRootSvelteDependencies
      √ should find svelte dependencies in packages/e2e-test/hmr
      √ should find nested svelte dependencies in packages/e2e-test/package-json-svelte-field (47 ms)

 PASS  packages/vite-plugin-svelte/src/utils/__tests__/sourcemap.spec.ts
  sourcemap
    buildMagicString
      √ should return a valid magic string
    buildSourceMap
      √ should return a map with mappings and filename

 PASS  packages/e2e-tests/configfile-esm/__tests__/configfile-esm.spec.ts (6.061 s)
  √ should load default config and work (68 ms)
  √ should load custom cjs config and work (1877 ms)

 PASS  packages/e2e-tests/configfile-custom/__tests__/configfile-custom.spec.ts (5.214 s)
  √ should load default config and work (37 ms)
  √ should load custom mjs config and work (1840 ms)

 PASS  packages/e2e-tests/ts-type-import/__tests__/ts-type-import.spec.ts
  √ should render App (22 ms)
  hmr
    √ should update App (256 ms)

 PASS  packages/e2e-tests/custom-extensions/__tests__/custom-extensions.spec.ts
  √ should render svg (59 ms)

Test Suites: 12 passed, 12 total
Tests:       59 passed, 59 total
Snapshots:   0 total
Time:        71.994 s
Ran all test suites.
Test results written to: temp\serve\jest-results.json
Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?

c:\Users\dominikg\develop\sveltejs\vite-plugin-svelte>

@patak-dev
Copy link
Member

Thanks for testing this @dominikg!

@bluwy bluwy deleted the fix/nested-optimization-windows branch September 1, 2021 12:06
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants