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: import.meta.glob support ?raw #5545

Merged
merged 7 commits into from
Dec 28, 2021
Merged

Conversation

ygj6
Copy link
Member

@ygj6 ygj6 commented Nov 4, 2021

Description

#5304 #3222
Add ?raw support for import.meta.globEager()
For example,

const files = import.meta.globEager('../assets/svgIcon/*.svg?raw')
console.log(files)
// Expected results
/*
{
  '../assets/svgIcon/github.svg': 
  <svg t="1634195145119" class="icon" viewBox="0 0 1024 1024" version="1.1" ...',
  '../assets/svgIcon/logo.svg': 
    '<svg t="1634195051463" class="icon" viewBox="0 0 1024 1024" version="1.1" ...'
}
*/

Additional context

  • Use cleanUrl() to remove ?raw so it will not affect glob.sync(pattern).
  • Whether need to add ?url support can be discussed

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.

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Nov 4, 2021
@ygj6
Copy link
Member Author

ygj6 commented Nov 13, 2021

I have implemented a version compatible with the standard. If it feels appropriate, I can continue to implement support such as url and json.

@patak-dev
Copy link
Member

Thanks! Lets wait til next week meeting so we check with Evan that this is the way to go

@patak-dev
Copy link
Member

We talked about it in the last team meeting, and we think it is good to align with the standard as this PR is now implemented. Thanks for the refactor @ygj6

For reference, these are notes from Evan showcasing the alignment. We could later add options like inline, exclude and others

import './foo.js' assert { type: "worker", inline: true }
import('./foo.svg', { assert: { type: "raw" }})
import.meta.glob('./*.svg', { assert: { type: "raw" }, exclude: '...' })

I'm worried about including json5 to do the parsing. But it is already in two of our dependencies (we will probably be using another version though). @bluwy @ygj6 what do you think?

Let's target the 2.8 beta to include this feature.

@patak-dev patak-dev added this to the 2.8 milestone Dec 4, 2021
@bluwy
Copy link
Member

bluwy commented Dec 4, 2021

I think json5 may be the most reliable choice for now, since the spec doesn't seem to be explicit on its syntax, so it would likely be parsing any kind of JS objects. +1 for import assertions though!

const commaIndex = options.indexOf(`,`)
let assert = {}
if (commaIndex > -1) {
assert = JSON5.parse(options.substr(commaIndex + 1))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert = JSON5.parse(options.substr(commaIndex + 1))
assert = JSON5.parse(options.substring(commaIndex + 1))

There was a recent PR that converted substr to substring. I think we can do the same here too.

@ygj6
Copy link
Member Author

ygj6 commented Dec 6, 2021

Thanks for your replies, I agree with @bluwy 's #5545 (comment) , I can continue to implement the rest later.

We could later add options like inline, exclude and others

@patak-js Where can I find reference docs about it to help me understand it more? thx

@patak-dev
Copy link
Member

Yes, let's merge this PR only with raw support in 2.8

@ygj6 the other types and options would resolve to the current suffixes (url, raw, worker, inline, etc). But this API isn't yet defined, maybe we could wait a bit here.

@ygj6
Copy link
Member Author

ygj6 commented Dec 7, 2021

@patak-js Oh, I see, looking forward to it coming. If there is anything I can help, please call me :)

In addition, I found some minor bugs in the code of lexGlobPattern

  1. when lexing the pattern, it may contain quote strings, e.g. '/Tom's files/**', results in '/Tom'
  2. simply find index of ) to get the endIndex is fragile, the options code may contain annotation, e.g.
import.meta.globEager('/dir/*.json', {
    // (set type)
    assert: { type: 'raw' }
  })

get the wrong pos next to type)<--

So, I plan to use acorn lexer to further refactor the code of lexGlobPattern and push it, see if you think the new approach is acceptable.

@bluwy
Copy link
Member

bluwy commented Dec 7, 2021

@ygj6 If we go with acorn, it might be the first time Vite does parsing in dev mode, which have been avoided for performance reasons. But given it only runs for globEager and glob, maybe it's not too bad. Otherwise we can accept this bugs upfront and document them instead to preserve performance. I'm slightly leaning on accepting the bugs though, as they can be easily worked around.

@patak-dev
Copy link
Member

Yes, as @bluwy said, we also discussed this with the team. We are accepting a few edge cases in exchange for speed with Vite. There are a lot of places where it will be better to remove regex and replace them with an AST but DX would suffer if we do that. It is a tradeoff that at this point is working well for us. So let's keep the current scheme and see if we can polish that edge cases or document the limitations if we can't.

@ygj6
Copy link
Member Author

ygj6 commented Dec 8, 2021

Reasonable, more understanding of vite’s pursuit of speed and certain tradeoffs, thanks for your patient explanation!

@boehs
Copy link
Contributor

boehs commented Dec 27, 2021

I think this is safe to merge?

@patak-dev patak-dev merged commit 5279de6 into vitejs:main Dec 28, 2021
poyoho added a commit to poyoho/vite that referenced this pull request Dec 30, 2021
commit d856c4b
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Thu Dec 30 00:25:59 2021 +0800

    fix(ssr): move `vite:ssr-require-hook` after user plugins (vitejs#6306)

commit b45f4ad
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Wed Dec 29 14:49:15 2021 +0100

    chore(deps): update all non-major dependencies (vitejs#6185)

commit 4d75b2e
Author: Niputi <7137178+Niputi@users.noreply.github.com>
Date:   Wed Dec 29 14:48:13 2021 +0100

    feat: catch postcss error messages (vitejs#6293)

commit f68ed8b
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Wed Dec 29 16:40:13 2021 +0300

    fix: replace execa with cross-spawn (vitejs#6299)

commit 44bb4da
Author: patak <matias.capeletto@gmail.com>
Date:   Wed Dec 29 12:51:46 2021 +0100

    chore(deps): update to esbuild fixed at 0.14.3 (vitejs#5861)

commit 9ad7c55
Author: patak <matias.capeletto@gmail.com>
Date:   Wed Dec 29 11:32:49 2021 +0100

    deps: update to typescript 4.5.4 (vitejs#6297)

commit 1da104e
Author: Aron Griffis <aron@scampersand.com>
Date:   Wed Dec 29 02:50:19 2021 -0500

    fix: don't force terser on non-legacy (fix vitejs#6266) (vitejs#6272)

commit 5279de6
Author: ygj6 <7699524+ygj6@users.noreply.github.com>
Date:   Wed Dec 29 05:30:47 2021 +0800

    feat: import.meta.glob support ?raw (vitejs#5545)

commit 6d4ee18
Author: Bjorn Lu <bjornlu.dev@gmail.com>
Date:   Wed Dec 29 04:27:49 2021 +0800

    feat(define): prevent assignment (vitejs#5515)

commit ac3f434
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Tue Dec 28 23:26:46 2021 +0300

    fix: upgrade postcss-modules (vitejs#6248)

commit 5a111ce
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Tue Dec 28 23:23:42 2021 +0300

    fix: replace chalk with picocolors (vitejs#6277)

commit 7e3e84e
Author: patak-dev <matias.capeletto@gmail.com>
Date:   Tue Dec 28 15:07:10 2021 +0100

    release: v2.7.9

commit 83ad7bf
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Tue Dec 28 21:52:41 2021 +0800

     fix: revert vitejs#6251 (vitejs#6290)

    This reverts commit 49da986.

commit 1cbf0e1
Author: Cristian Pallarés <cristian@pallares.io>
Date:   Tue Dec 28 11:30:42 2021 +0100

    test: fix test typo (vitejs#6285)

commit d13ced5
Author: patak-dev <matias.capeletto@gmail.com>
Date:   Tue Dec 28 09:40:48 2021 +0100

    release: v2.7.8

commit dcb1df4
Author: itibbers <jxn2014@gmail.com>
Date:   Tue Dec 28 16:30:32 2021 +0800

    docs: add frontmatters to fix __VP_STATIC_START__ (vitejs#6283)

commit 60ce7f9
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Tue Dec 28 16:20:12 2021 +0800

    fix(ssr): capture scope declaration correctly (vitejs#6281)

commit eb08ec5
Author: Niputi <7137178+Niputi@users.noreply.github.com>
Date:   Tue Dec 28 06:10:37 2021 +0100

    chore: remove acorn plugins (vitejs#6275)

commit 64b1595
Author: zhangenming <282126346@qq.com>
Date:   Tue Dec 28 11:52:51 2021 +0800

    chore(create-vite): add more gitignore (vitejs#6247)

commit 49da986
Author: sanyuan <39261479+sanyuan0704@users.noreply.github.com>
Date:   Tue Dec 28 05:30:54 2021 +0800

    fix: seperate source and dep for dymamic import after build (vitejs#6251)

commit 394539c
Author: Bogdan Chadkin <trysound@yandex.ru>
Date:   Tue Dec 28 00:29:23 2021 +0300

    fix: upgrade to launch-editor with picocolors (vitejs#6209)

commit 40e3f73
Author: Shinigami <chrissi92@hotmail.de>
Date:   Mon Dec 27 11:42:24 2021 +0100

    chore: fix link (vitejs#6269)

commit e7306b5
Author: Shinigami <chrissi92@hotmail.de>
Date:   Mon Dec 27 11:22:44 2021 +0100

    chore: update bug report issue template (vitejs#6263)

commit 1f945f6
Author: Aaron Bassett <arbassett4@outlook.com>
Date:   Sun Dec 26 15:28:47 2021 -0500

    fix(html): show error overlay when parsing invalid file (vitejs#6184)

commit 1d722c5
Author: patak-dev <matias.capeletto@gmail.com>
Date:   Sun Dec 26 06:35:04 2021 +0100

    release: v2.7.7

commit 2e3fe59
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Sun Dec 26 13:13:24 2021 +0800

    fix(ssr): transform class props (vitejs#6261)

commit 1a6e2da
Author: ygj6 <7699524+ygj6@users.noreply.github.com>
Date:   Sat Dec 25 18:24:44 2021 +0800

    docs: typescript tips for using Type-Only Imports and Export (vitejs#6260)

commit 6a47083
Author: Haoqun Jiang <haoqunjiang@gmail.com>
Date:   Fri Dec 24 14:02:43 2021 +0800

    fix: update the vue version in the error message (vitejs#6252)

commit 485e298
Author: Anthony Fu <anthonyfu117@hotmail.com>
Date:   Thu Dec 23 21:45:13 2021 +0800

    fix(ssr): nested destucture (vitejs#6249)
@jd-solanki
Copy link

I am using vite 2.8.0-beta.1 but with below code I am unable to get it working 🤔

const pageLayoutsGlob = import.meta.globEager('../../../views/generator/sneat/page-layouts/*.html?raw')
console.log('pageLayouts :>> ', pageLayoutsGlob)

Screenshot 2022-01-11 at 6 11 41 PM

Does anyone know why?

@jd-solanki
Copy link

Same if I use import.meta.glob()

@patak-dev
Copy link
Member

@jd-0001 you should use

const pageLayoutsGlob = import.meta.globEager('../../../views/generator/sneat/page-layouts/*.html',{ assert: { type: 'raw' }})
console.log('pageLayouts :>> ', pageLayoutsGlob)

@jd-solanki
Copy link

Thanks :)

Just to make vite robust I have something to share:

const pageLayoutsGlob = import.meta.globEager('../../../views/generator/sneat/page-layouts/*.html',{ assert: { type: 'raw' }})
console.log('pageLayouts :>> ', pageLayoutsGlob)

won't work because I guess it is taking path from root dir of project.

Hence, I updated it to:

const pageLayoutsGlob = import.meta.globEager('/src/views/generator/sneat/page-layouts/*.html', {
  assert: { type: 'raw' },
})
console.log('pageLayouts :>> ', pageLayoutsGlob)

Also, I am getting linting errors:
Screenshot 2022-01-12 at 12 16 49 PM

I guess function signature isn't updated yet 🤔
Screenshot 2022-01-12 at 12 17 21 PM

I am using vite vite@2.8.0-beta.1

@Shinigami92
Copy link
Member

I assume #6433 is not released yet

@ygj6
Copy link
Member Author

ygj6 commented Jan 24, 2022

@LiuJiangshan Would you like to open a new issue and describe the request more clearly in it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants