Skip to content

Comments

feat: support globbing from dependencies#2519

Merged
patak-dev merged 1 commit intovitejs:mainfrom
brillout:fix-2390
Apr 11, 2021
Merged

feat: support globbing from dependencies#2519
patak-dev merged 1 commit intovitejs:mainfrom
brillout:fix-2390

Conversation

@brillout
Copy link
Contributor

@brillout brillout commented Mar 15, 2021

Fixes #2390

The condition source.includes('import.meta.glob') won't catch source code like import.meta['glob'] or new lines between meta and glob, but the subsequent transform() code won't either anyways.

        const { s: start, e: end, ss: expStart, d: dynamicIndex } = imports[
          index
        ]

        // This as well doesn't catch `import.meta['glob']` 
        const isGlob =
          source.slice(start, end) === 'import.meta' &&
          source.slice(end, end + 5) === '.glob'

The advantage of source.includes('import.meta.glob') is that we skip es-module-lexer parsing.

@brillout brillout force-pushed the fix-2390 branch 2 times, most recently from 8d459e3 to f7cd2ba Compare March 17, 2021 21:19
This was referenced Mar 17, 2021
Closed
Closed
Closed
@brillout
Copy link
Contributor Author

brillout commented Mar 19, 2021

It's high prio for vite-plugin-ssr. I wrote a patch that simulates this PR (https://github.com/brillout/vite-fix-2390) but it's painful to make sure that the patch works against each release.

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Mar 19, 2021
@patak-dev
Copy link
Member

patak-dev commented Mar 19, 2021

This looks good to me, but I think @yyx990803 explained in an issue why globbing from dependencies was not a good idea (but I can not find it). We'll need to wait for his input about this one.

@brillout
Copy link
Contributor Author

brillout commented Mar 19, 2021

There was a previous request for adding support for globbing from dependencies at #1875 which got fixed by Evan You. So I'm assuming this PR would get accepted as well.

@brillout
Copy link
Contributor Author

brillout commented Mar 22, 2021

@matias-capeletto Maybe we should label this "p3-downstream-blocker" since it's blocking vite-plugin-ssr.

@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Mar 22, 2021
@patak-dev patak-dev requested a review from yyx990803 March 22, 2021 09:19
@patak-dev
Copy link
Member

patak-dev commented Mar 22, 2021

@brillout could we add a test for this feature?

@brillout
Copy link
Contributor Author

brillout commented Mar 24, 2021

@matias-capeletto Yes! That would actually be great to solidify that use case. I'll make a stab at it (I'm bit woried about how I can use a non-symlinked dependency with the current test architecture, but I'll dig into this). Sorry for the late reply, I was heads down implementing Client-side Routing for vite-plugin-ssr.

Shinigami92
Shinigami92 previously approved these changes Mar 27, 2021
View reviewed changes
@brillout
Copy link
Contributor Author

brillout commented Mar 27, 2021

@patak-js I'm almost done with the test. I expect the test to get some back-and-forth with the reviewers so I'm a bit worried that it may delay the merging of this PR. If it's ok with you I will open a second PR for the test so that this PR can get merged quicklier. Please let me know if you object.

@patak-dev
Copy link
Member

patak-dev commented Mar 27, 2021

I am fine with that, but this counts as a new feature so we need @yyx990803 approval to merge it.

@brillout
Copy link
Contributor Author

brillout commented Mar 27, 2021

I just changed the commit prefix from fix: to feat:. So I guess Shinigami92's approval still holds.

@brillout
Copy link
Contributor Author

brillout commented Mar 27, 2021

I'm almost done with the test

Done: #2740.

@antfu antfu changed the title fix: support globbing from dependencies feat: support globbing from dependencies Mar 31, 2021
@brillout brillout mentioned this pull request Apr 1, 2021
Merged
@ryansolid
Copy link

ryansolid commented Apr 5, 2021 •
edited
Loading

I hit this too building the Solid Starter. For now patching it but look forward to a solution being merged. This seems pretty essential.

@patak-dev patak-dev mentioned this pull request Apr 9, 2021
Merged
9 tasks
@patak-dev patak-dev merged commit 7121553 into vitejs:main Apr 11, 2021
@samuveth samuveth mentioned this pull request Jul 19, 2021
Merged
7 tasks
@brillout brillout deleted the fix-2390 branch September 18, 2024 08:15
@sapphi-red sapphi-red mentioned this pull request May 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yyx990803 yyx990803 yyx990803 approved these changes

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

+1 more reviewer

@Shinigami92 Shinigami92 Shinigami92 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

import.meta.glob() not processed if living in node_modules/

5 participants

@brillout @patak-dev @ryansolid @yyx990803 @Shinigami92