Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

fix(nuxt): do not apply import protection to top-level resolution #7344

Merged
merged 3 commits into from
Sep 10, 2022

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

resolves nuxt/nuxt#13278

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

We can avoid triggering import protection from a top-level this.resolve call (i.e. within another vite plugin), as there may be valid use cases for this, like tailwind resolving the nuxt.config file.

Relevant vite section:

https://github.com/vitejs/vite/blob/757a92f1c7c4fa961ed963edd245df77382dfde6/packages/vite/src/node/server/pluginContainer.ts#L562-L566

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added bug Something isn't working 🍰 p2-nice-to-have Priority 2: nothing is broken but it's worth addressing labels Sep 8, 2022
@danielroe danielroe requested a review from pi0 September 8, 2022 09:22
@danielroe danielroe self-assigned this Sep 8, 2022
@netlify
Copy link

netlify bot commented Sep 8, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit 240bdb4
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/631c72bca2daed0008c2c311

@pi0
Copy link
Member

pi0 commented Sep 8, 2022

Thanks! This defenitly reduces some frustrations but why should a plugin attempt to import from nuxt.config or other potentially build-only files that we need to disable protection against? (i think tailwind for runtime config generates a virtual file)

@pi0 pi0 mentioned this pull request Sep 9, 2022
@danielroe
Copy link
Member Author

danielroe commented Sep 9, 2022

I am not sure exactly how to answer that. Tailwind on its own is triggering this import protection rule. (ie. I'm reproducing it with pure tailwind, not the nuxt integration).

note: the import protection rule triggers on resolving an id, not on importing from it.

@pi0
Copy link
Member

pi0 commented Sep 10, 2022

I understand issue with our impl and tailwind but the change of this PR is also defusing the import protection for nuxt.config at all which was the initial point! e.g: Users now can directly import something from nuxt.config in app.vue.

@pi0
Copy link
Member

pi0 commented Sep 10, 2022

note: the import protection rule triggers on resolving an id, not on importing from it.

What about marking triggered resolutions (resolveId) and then show error in transform step if id actually resolve?

@pi0 pi0 added the pending label Sep 10, 2022
@danielroe
Copy link
Member Author

What makes you say they can import in app.vue?

@pi0
Copy link
Member

pi0 commented Sep 10, 2022

It won't work because there is another import protection rule for defineNuxtConfig from nuxt. Directly returning config or import from any top level will be bypassed.

@danielroe
Copy link
Member Author

danielroe commented Sep 10, 2022

Just did a quick test to confirm (thought I had done it when creating PR but always worth checking!) and indeed import protection works correctly in app.vue. The plugin gets called with an importer of app.vue. The change in this PR only applies to top-level resolutions (ie, from index.html).

You can confirm with:

<script setup lang="ts">
import config from '~/nuxt.config'
console.log({ config })
</script>

<template>
  <!-- Edit this file to play around with Nuxt but never commit changes! -->
  <div>
    Nuxt 3 Playground
  </div>
</template>

<style scoped>
</style>

What makes you say they can import in app.vue?

(The reason I asked is because I was wondering if you had done some testing to make you think the protection wasn't still working properly. If so, a reproduction would be helpful.)

@pi0
Copy link
Member

pi0 commented Sep 10, 2022

nuxt.config:

export const a = 123

export default {}

app.vue:

<script setup lang="ts">
import { a } from '~/nuxt.config'
console.log(a)
</script>

(works with no warning)

@danielroe
Copy link
Member Author

danielroe commented Sep 10, 2022

That's not caused by this PR - it's a bug with the import protection rules. Pushing fix.

To show that imports from app.vue remain protected, you can also test with:

<script setup lang="ts">
import { useNuxtApp } from 'nuxt/app'
console.log({ useNuxtApp })
</script>

@pi0
Copy link
Member

pi0 commented Sep 10, 2022

Before your last commit, protection works without change to exclude top level index.html and was not working after adding exclude. Now ~/nuxt.config is working (thanks) but import { a } from './nuxt.config' not protected but this one was happening before too. Do you think we fix it as well?

@danielroe
Copy link
Member Author

I think so. πŸ‘ Will have a look.

@pi0 pi0 removed the pending label Sep 10, 2022
@pi0 pi0 merged commit 9abc7a2 into main Sep 10, 2022
@pi0 pi0 deleted the fix/top-level-protection branch September 10, 2022 11:51
@pi0 pi0 mentioned this pull request Sep 15, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.x bug Something isn't working 🍰 p2-nice-to-have Priority 2: nothing is broken but it's worth addressing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tailwind triggers nuxt.config import protection
2 participants