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

Jump to bottom

fix: glob import parsing (#10949) #11056

Merged
merged 5 commits into from Nov 28, 2022
Merged

fix: glob import parsing (#10949) #11056

merged 5 commits into from Nov 28, 2022

Conversation

gtm-nayan
Copy link
Contributor

Description

closes #10949
supersedes/closes #11051

I was originally planning on just using findNodeAt but then noticed that it's using strip-literal which IIUC also tries to parse the file. Figured why not do it with a single AST pass.

Additional context

Keeping as draft for now because acorn-walk is giving a type error for the build script

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.

@bluwy
Copy link
Member

bluwy commented Nov 25, 2022

I was originally planning on just using findNodeAt but then noticed that it's using strip-literal which IIUC also tries to parse the file. Figured why not do it with a single AST pass.

strip-literal isn't exactly parsing but only tokenizing the code, so we should probably keep it like before so we don't have to parse the entire code.

@gtm-nayan
Copy link
Contributor Author

Ahh ok, I thought it was parsing and removing the nodes from the description, will revert and go with findNodeAt then,

I need some help with the types problem though: the build-types script is failing for acorn-walk currently, I'm thinking it's because it imports a type from acorn in its declaration but doesn't actually list acorn as a dependency or dev dependency. My editor seems to be fine with it but tsc refuses to build. Any ideas?

@bluwy
Copy link
Member

bluwy commented Nov 28, 2022

Hmm yeah we got this issue before with postcss-load-config but was fixed upstream. I think we could configure https://pnpm.io/package_json#pnpmpackageextensions to add acorn as a dependency for acorn-walk to workaround this.

@gtm-nayan gtm-nayan marked this pull request as ready for review November 28, 2022 08:26
bluwy
bluwy approved these changes Nov 28, 2022
View reviewed changes
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.

Nice! For other reviewers, findNodeAt is fairly small so it shouldn't increase the bundle size by much.

@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 28, 2022
@patak-dev patak-dev merged commit ac2cfd6 into vitejs:main Nov 28, 2022
@bluwy bluwy mentioned this pull request Nov 29, 2022
Merged
bluwy pushed a commit that referenced this pull request Dec 5, 2022
@gtm-nayan @bluwy
fix: glob import parsing (#10949) (#11056)
e18bf08 
closes #10949
closes #11051
@bluwy bluwy mentioned this pull request Dec 5, 2022
Merged
patak-dev pushed a commit that referenced this pull request Dec 5, 2022
@bluwy @gtm-nayan
@julienv3 @sapphi-red
chore: cherry pick more v4 bug fixes to v3 (#11189)
eba9b42 
* fix: glob import parsing (#10949) (#11056)

closes #10949
closes #11051

* fix: import.meta.env and process.env undefined variable replacement (fix #8663) (#10958)

Co-authored-by: bluwy <bjornlu.dev@gmail.com>
fix #8663

* fix(esbuild): handle inline sourcemap option (#11120)

* fix(importGlob): preserve line count for sourcemap (#11122)

* fix: Dev SSR dep optimization + respect optimizeDeps.include (#11123)

* fix: reset global regex before match (#11132)

* chore: fix test

Co-authored-by: gtmnayan <50981692+gtm-nayan@users.noreply.github.com>
Co-authored-by: julienv3 <julienv3@gmail.com>
Co-authored-by: 翠 / green <green@sapphi.red>
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@gtm-nayan @futurGH
fix: glob import parsing (vitejs#10949) (vitejs#11056)
1b93232 
closes vitejs#10949
closes vitejs#11051
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patak-dev patak-dev patak-dev approved these changes

@bluwy bluwy bluwy approved these changes

Assignees
No one assigned
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

import.meta.glob breaks in Svelte template
3 participants
@gtm-nayan @bluwy @patak-dev