Skip to content

fix: glob import parsing (#10949)#11056

Merged
patak-cat merged 5 commits intovitejs:mainfrom
gtm-nayan:better-glob-import-parsing
Nov 28, 2022
Merged

fix: glob import parsing (#10949)#11056
patak-cat merged 5 commits intovitejs:mainfrom
gtm-nayan:better-glob-import-parsing

Conversation

@gtm-nayan
Copy link
Contributor

@gtm-nayan gtm-nayan commented Nov 24, 2022

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

gtm-nayan commented Nov 25, 2022

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?

@gtm-nayan gtm-nayan force-pushed the better-glob-import-parsing branch from 6091a08 to a53b633 Compare November 25, 2022 11:53
@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 force-pushed the better-glob-import-parsing branch from b25e883 to 695c80d Compare November 28, 2022 08:14
@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-cat patak-cat 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-cat 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-cat patak-cat patak-cat 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-cat