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(helpers): Handle Vite config objects declared as variables in addVitePlugin #69

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

Lms24
Copy link
Contributor

@Lms24 Lms24 commented Jul 26, 2023

🔗 Linked issue

This sort of addresses #68 but only for addVitePlugin and in a less universal way. Still happy to work on #68 if the proposal is accepted but I'd like to contribute this fix first so that we can unblock our usage of magicast.

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR adds support for handling vite configs that are not directly exported as default objects in addVitePlugin.

Currently, a code snippet like

// vite.config.js
const config = {
  plugins: [somePlugin()]
}
export default config;

causes addVitePlugin to throw with

TypeError: 'set' on proxy: trap returned falsish for property 'plugins'

With this PR, we take the identifier for the default export (if it exists), look up its declaration and if we find it, modify it. Admittedly, the modification and writing back part of this fix is a little messy. With my limited knowledge of the codebase, I think this is as much as we can do, until we come up with a better, general API for modifying declarations (see #68). Happy to be proven wrong though :) (i might need some guidance in that case to do better).

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.
    Happy to update the readme but first I'd like to know if this has potential to be merged ;)

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #69 (31b9019) into main (d9ef2eb) will increase coverage by 0.14%.
The diff coverage is 94.40%.

@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
+ Coverage   92.17%   92.31%   +0.14%     
==========================================
  Files          24       24              
  Lines        1623     1731     +108     
  Branches      301      315      +14     
==========================================
+ Hits         1496     1598     +102     
- Misses        127      133       +6     
Files Changed Coverage Δ
src/helpers/config.ts 89.85% <89.39%> (+4.14%) ⬆️
src/helpers/vite.ts 96.75% <100.00%> (+1.38%) ⬆️

@Lms24 Lms24 changed the title feat: Handle Vite config objects declared as variables in addVitePlugin feat(helpers): Handle Vite config objects declared as variables in addVitePlugin Jul 26, 2023
@antfu antfu merged commit 224eed2 into unjs:main Jul 27, 2023
@Lms24 Lms24 deleted the lms/add-vite-plugin-variable-declaration branch July 27, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants