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

Add support for eager via the { eager: true } argument to import.meta.glob #43

Merged

Conversation

jazlalli
Copy link

@jazlalli jazlalli commented Nov 3, 2023

🙋🏽 Question

I've targeted the code as specified at https://vitejs.dev/guide/features.html#glob-import, but realise that that might not be what's needed in a Node env, can someone confirm? Should the code produced actually use require()?

What:

Addresses #42

Adds support for specifying eager import via the ImportGlobOptions argument to import.meta.glob

Why:

import.meta.globEager is deprecated in favour of the options argument. Without support in this plugin, Babel (and therefore Jest) is unable to understand this syntax.

How:

The target for what this should produce was obtained from the Vite docs on Glob Imports. Specifically,

If you'd rather import all the modules directly (e.g. relying on side-effects in these modules to be applied first), you can pass { eager: true } as the second argument:

const modules = import.meta.glob('./dir/*.js', { eager: true })

The above will be transformed into the following:

// code produced by vite
import * as __glob__0_0 from './dir/foo.js'
import * as __glob__0_1 from './dir/bar.js'
const modules = {
  './dir/foo.js': __glob__0_0,
  './dir/bar.js': __glob__0_1,
}

A new visitor is added to target the entire VariableDeclaration, and it is replaced with the specified code above.

Checklist:

  • Documentation updated
  • Tests
  • Ready to be merged
  • Added myself to contributors table - the npm run contributors:add command is not available, and I didn't want to manually modify the contributors sections, seeing as it says not to 🙃

Jaz Lalli added 3 commits November 3, 2023 14:53
According to Vite types, globEager is deprecated in favour of supplying an
options argument to glob (from importMeta.d.ts 👇🏽)

```
/**
 * @deprecated Use `import.meta.glob('*', { eager: true })` instead
 */
globEager: import('./importGlob').ImportGlobEagerFunction
```

This commit adds tests cases to cover the desired behaviour, and the
appropriate implementation. The target code was obtained from the Vite
docs at [1]. This only supports the eager option, others are not covered
here.

[1] https://vitejs.dev/guide/features.html#glob-import:~:text=%7B%20eager%3A%20true%20%7D)-,The%20above%20will%20be%20transformed%20into%20the%20following%3A,-js

path.replaceWith(replacement)
}
}
Copy link
Author

Choose a reason for hiding this comment

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

This can def be tidied up, but will wait until we're sure that this is producing the right/desired result before doing so

@jazlalli
Copy link
Author

jazlalli commented Nov 4, 2023

For the record, in my project where tests previously resulted in SyntaxError: Cannot use 'import.meta' outside a module, after installing from this branch

// package.json
"babel-plugin-transform-vite-meta-glob": "https://gitpkg.now.sh/jazlalli/babel-vite/packages/babel-plugin-transform-vite-meta-glob?pr/support-eager-glob-option"`

and adding the plugin into config

// babel.config.js
plugins: ["babel-plugin-transform-vite-meta-glob"],

All tests run successfully and pass 🟢

@vctqs1
Copy link
Contributor

vctqs1 commented Dec 12, 2023

@mpeyper Hi, could you help review this PR. I need this PR to upgrade vite@v5

@mpeyper
Copy link
Member

mpeyper commented Dec 12, 2023

Hey, sorry, github notifcations are rubbish and I completely missed this one (and the issue it references).

It's been a long time since I've looked at the code here, so I'm feeling very rusty and I don't really have the bandwidth at the moment to dig into it all again. That said, the change looks ok to me, especially the snapshot output from the tests, however, I can't see the button to approve the checks to run (It's probably been too long since the commits were made). @jazlalli, if you are still interested in getting this through, could you bump a new commit onto the branch and see if checks come back to life?

@JacobMGEvans
Copy link
Member

Hey, sorry, github notifcations are rubbish and I completely missed this one (and the issue it references).

It's been a long time since I've looked at the code here, so I'm feeling very rusty and I don't really have the bandwidth at the moment to dig into it all again. That said, the change looks ok to me, especially the snapshot output from the tests, however, I can't see the button to approve the checks to run (It's probably been too long since the commits were made). @jazlalli, if you are still interested in getting this through, could you bump a new commit onto the branch and see if checks come back to life?

Wild, I also didn't get notifications and its my Organization lmao thanks GitHub...

@JacobMGEvans JacobMGEvans reopened this Dec 12, 2023
@JacobMGEvans JacobMGEvans added Status: In Progress 🚧 Currently taking out the trash mobs or cleaning up the dungeon Status: Review labels Dec 12, 2023
@vctqs1
Copy link
Contributor

vctqs1 commented Dec 14, 2023

@mpeyper Could you help check again, I see @jazlalli fork and commit on last month (Nov), while the latest commit on repo is 8 months ago. I think the PR already included the latest commits

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (883dabd) 100.00% compared to head (855a136) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #43   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines           93       130   +37     
  Branches        32        49   +17     
=========================================
+ Hits            93       130   +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JacobMGEvans JacobMGEvans merged commit e8dbeb9 into OpenSourceRaidGuild:main Dec 14, 2023
4 checks passed
@mpeyper
Copy link
Member

mpeyper commented Dec 14, 2023

It looks like the action failed to release this. I think this project needs a bit of TLC.

@vctqs1
Copy link
Contributor

vctqs1 commented Dec 16, 2023

@JacobMGEvans Could you help review this PR #46, just to upgrade node to 20. Also run test on CI in fork repo for sure

Copy link

🎉 This PR is included in version 1.1.1 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version 1.1.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@vctqs1
Copy link
Contributor

vctqs1 commented Dec 18, 2023

I got some issue with jest if use import.meta.glob with second args {eager: true} everything will be fine if I removed second args cc @jazlalli
image

@mpeyper
Copy link
Member

mpeyper commented Dec 18, 2023

@vctqs1 I’m not sure how including the second argument could break jest parsing of es module syntax? Can you share a repo and open an issue for it?

@vctqs1
Copy link
Contributor

vctqs1 commented Dec 21, 2023

sure, let me create sandbox

@vctqs1
Copy link
Contributor

vctqs1 commented Dec 26, 2023

@mpeyper sorry for my late response, here is my sandbox
to test w this repo, please run npm test
also I put the import.meta.glob in HelloWorld.tsx


const all = import.meta.glob("./*.ts", {
  eager: true,
});

without second args
image

with second args
image

@vctqs1
Copy link
Contributor

vctqs1 commented Dec 26, 2023

@mpeyper I did a fix for above comment here #51 , could you please have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Status: In Progress 🚧 Currently taking out the trash mobs or cleaning up the dungeon Status: Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants