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

docs: clarify code comments in optimize-missing-deps #7332

Merged
merged 7 commits into from
May 4, 2022

Conversation

benmccann
Copy link
Collaborator

Description

I didn't understand that "missing" meant not discovered. Also, the sentence "this will import missing deps nest built-in deps" seems like it has a typo because it doesn't make sense

Please check that what I've written is correct

Additional context

This test was failing when I was working on another change and I wanted to better understand it


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.

ygj6
ygj6 previously approved these changes Mar 28, 2022
Copy link
Member

@ygj6 ygj6 left a comment

Choose a reason for hiding this comment

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

I'm sorry I wrote this confusing note, which contains a stale story about "built-in deps". Your changes are correct to make it clearer, thank you!

In addition, the main purpose of optimize-missing-deps is to test that _registerMissingImport is not triggered during SSR. You can see the reason and my detailed description at #5017 (comment). Hope it helps you understand.

brillout
brillout previously approved these changes May 3, 2022
@brillout
Copy link
Contributor

brillout commented May 3, 2022

@ygj6 Do you know why the test is failing (https://github.com/vitejs/vite/pull/6698/checks) when externalizing SSR dependencies (#6698)?

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.

Wonder if this description is more concise. I find the current one a bit off 😅

packages/playground/optimize-missing-deps/server.js Outdated Show resolved Hide resolved
packages/playground/optimize-missing-deps/server.js Outdated Show resolved Hide resolved
@benmccann benmccann dismissed stale reviews from brillout and ygj6 via 05bd213 May 3, 2022 14:39
@bluwy bluwy added documentation Improvements or additions to documentation p1-chore Doesn't change code behavior (priority) labels May 3, 2022
@patak-dev patak-dev merged commit 70f032f into vitejs:main May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants