-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix: opt in to import.meta.*
properties
#2622
Conversation
✅ Deploy Preview for pinia-playground canceled.
|
✅ Deploy Preview for pinia-official ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## v2 #2622 +/- ##
=======================================
Coverage 95.42% 95.42%
=======================================
Files 11 11
Lines 2886 2886
Branches 190 190
=======================================
Hits 2754 2754
Misses 131 131
Partials 1 1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think it's fine not to add a peerdep version as it's broken things with npm in the past and I think 3.7 was released a while ago
* docs: project sync init * docs: zh docs delete and some docs translate using gpt-4-turbo * docs: overlay * docs: markup * fix: opt in to `import.meta.*` properties (vuejs#2622) * docs: certificate links
@posva I think this PR broke pinia for Nuxt Bridge
Can conffirm reverting to |
@@ -16,7 +16,7 @@ export default (context: any, provide: any) => { | |||
Object.defineProperty(store, '$nuxt', { value: context }) | |||
}) | |||
|
|||
if (process.server) { | |||
if (import.meta.server) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielroe Do you think we should just keep process.server
for Vue 2? I suppose it won't be added to nuxt bridge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't already, Nuxt bridge should support import.meta. If someone can open a bridge repro we can fix there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to do that tomorrow if it hasn't been done already
This is a very early PR to make this module compatible with changes we expect to release in Nuxt v5.
In Nuxt v3.7.0 we added support for
import.meta.*
(see original PR) and we've been gradually updating docs and moving across from the oldprocess.*
patterned variables.As I'm sure you're aware, these variables are replaced at build-time and enable tree-shaking in bundled code.
This change affects runtime code (that is, that is processed by the Nuxt bundler, like vite or webpack) rather than code running in Node. So it really doesn't matter what the string is, but it makes more sense in an ESM-world to use
import.meta
rather thanprocess
.(It might be worth updating the module compatibility as well to indicate it needs to have Nuxt v3.7.0+, but I'll leave that with you if you think this is a good approach.)