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

feat: support globbing from dependencies #2519

Merged
merged 1 commit into from
Apr 11, 2021
Merged

Conversation

brillout
Copy link
Contributor

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
Copy link
Contributor Author

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

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

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

@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
Copy link
Member

@brillout could we add a test for this feature?

@brillout
Copy link
Contributor Author

@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
@brillout
Copy link
Contributor Author

@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

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

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

@brillout
Copy link
Contributor Author

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
@ryansolid
Copy link

ryansolid commented Apr 5, 2021

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
9 tasks
@patak-dev patak-dev merged commit 7121553 into vitejs:main Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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