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

fix: do not inject imports within node_modules #873

Merged
merged 2 commits into from
Aug 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,28 @@ jobs:
- name: Test (fixtures with dev)
run: pnpm test:fixtures:webpack:dev

test-unit:
runs-on: ${{ matrix.os }}

strategy:
matrix:
os: [ubuntu-latest, windows-latest]
node: [16]

steps:
- uses: actions/checkout@v3
- run: corepack enable
- uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node }}
cache: "pnpm"

- name: Install dependencies
run: pnpm install

- name: Test (Unit)
run: pnpm test:unit

build-release:
permissions:
id-token: write
Expand All @@ -130,6 +152,7 @@ jobs:
- build
- test-fixtures
- test-fixtures-webpack
- test-unit
runs-on: ${{ matrix.os }}

strategy:
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"test:fixtures": "pnpm dev:prepare && JITI_ESM_RESOLVE=1 vitest run --dir test",
"test:fixtures:dev": "NUXT_TEST_DEV=true pnpm test:fixtures",
"test:fixtures:webpack": "TEST_WITH_WEBPACK=1 pnpm test:fixtures",
"test:fixtures:webpack:dev": "TEST_WITH_WEBPACK=1 NUXT_TEST_DEV=true pnpm test:fixtures"
"test:fixtures:webpack:dev": "TEST_WITH_WEBPACK=1 NUXT_TEST_DEV=true pnpm test:fixtures",
"test:unit": "vitest run --dir packages"
},
"devDependencies": {
"@nuxt/test-utils": "^3.6.5",
Expand Down
1 change: 1 addition & 0 deletions packages/bridge/src/imports/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export default defineNuxtModule<Partial<ImportsOptions>>({
imports: [],
dirs: [],
transform: {
include: [],
exclude: undefined
}
},
Expand Down
22 changes: 17 additions & 5 deletions packages/bridge/src/imports/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ import { createUnplugin } from 'unplugin'
import { parseQuery, parseURL } from 'ufo'
import { Unimport } from 'unimport'
import { ImportsOptions } from '@nuxt/schema'
import { normalize } from 'pathe'

const NODE_MODULES_RE = /[\\/]node_modules[\\/]/
const IMPORTS_RE = /(['"])#imports\1/

export const TransformPlugin = createUnplugin(({ ctx, options }: { ctx: Unimport, options: Partial<ImportsOptions> }) => {
return {
Expand All @@ -12,10 +16,12 @@ export const TransformPlugin = createUnplugin(({ ctx, options }: { ctx: Unimport
const { pathname, search } = parseURL(decodeURIComponent(pathToFileURL(id).href))
const { type, macro } = parseQuery(search)

const exclude = options.transform?.exclude || [/[\\/]node_modules[\\/]/]

// Exclude node_modules by default
if (exclude.some(pattern => id.match(pattern))) {
// Included
if (options.transform?.include?.some(pattern => pattern.test(id))) {
return true
}
// Excluded
if (options.transform?.exclude?.some(pattern => pattern.test(id))) {
return false
}

Expand All @@ -33,7 +39,13 @@ export const TransformPlugin = createUnplugin(({ ctx, options }: { ctx: Unimport
}
},
async transform (_code, id) {
const { code, s } = await ctx.injectImports(_code)
id = normalize(id)
const isNodeModule = NODE_MODULES_RE.test(id) && !options.transform?.include?.some(pattern => pattern.test(id))
// For modules in node_modules, we only transform `#imports` but not doing imports
if (isNodeModule && !IMPORTS_RE.test(_code)) {
return
}
const { code, s } = await ctx.injectImports(_code, id, { autoImport: options.autoImport && !isNodeModule })
if (code === _code) {
return
}
Expand Down
74 changes: 74 additions & 0 deletions packages/bridge/test/auto-imports.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { describe, expect, it } from 'vitest'
import * as VueFunctions from 'vue'
import type { Import } from 'unimport'
import { createUnimport } from 'unimport'
import type { Plugin } from 'vite'
import { TransformPlugin } from '../src/imports/transform'
import { defaultPresets } from '../src/imports/presets'

describe('imports:transform', () => {
const imports: Import[] = [
{ name: 'ref', as: 'ref', from: 'vue' },
{ name: 'computed', as: 'computed', from: 'bar' },
{ name: 'foo', as: 'foo', from: 'excluded' }
]

const ctx = createUnimport({
imports
})

const transformPlugin = TransformPlugin.raw({ ctx, options: { transform: { exclude: [/node_modules/] } } }, { framework: 'rollup' }) as Plugin
const transform = async (source: string) => {
const result = await (transformPlugin.transform! as Function).call({ error: null, warn: null } as any, source, '')
return typeof result === 'string' ? result : result?.code
}

it('should correct inject', async () => {
expect(await transform('const a = ref(0)')).toMatchInlineSnapshot('"import { ref } from \'vue\';\nconst a = ref(0)"')
})

it('should ignore existing imported', async () => {
expect(await transform('import { ref } from "foo"; const a = ref(0)')).to.equal(undefined)
expect(await transform('import { computed as ref } from "foo"; const a = ref(0)')).to.equal(undefined)
expect(await transform('import ref from "foo"; const a = ref(0)')).to.equal(undefined)
expect(await transform('import { z as ref } from "foo"; const a = ref(0)')).to.equal(undefined)
expect(await transform('let ref = () => {}; const a = ref(0)')).to.equal(undefined)
expect(await transform('let { ref } = Vue; const a = ref(0)')).to.equal(undefined)
expect(await transform('let [\ncomputed,\nref\n] = Vue; const a = ref(0); const b = ref(0)')).to.equal(undefined)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fix #33?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't reproduce it and don't know what caused it, so I don't know if it will be fixed...


it('should ignore comments', async () => {
const result = await transform('// import { computed } from "foo"\n;const a = computed(0)')
expect(result).toMatchInlineSnapshot(`
"import { computed } from 'bar';
// import { computed } from \\"foo\\"
;const a = computed(0)"
`)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worthwhile to add another assertion here that it can ignore block style comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is worthwhile if block style comments are not ignored.


it('should exclude files from transform', async () => {
expect(await transform('excluded')).toEqual(undefined)
})
})

const excludedVueHelpers = [
'mergeDefaults',
'version',
'warn',
'watchPostEffect',
'watchSyncEffect',
'set',
'del',
'default'
]

describe('imports:vue', () => {
for (const name of Object.keys(VueFunctions)) {
if (excludedVueHelpers.includes(name)) {
continue
}
it(`should register ${name} globally`, () => {
expect(defaultPresets.find(a => a.from === 'vue')!.imports).toContain(name)
})
}
})
Loading