Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

feat: add traceWithNft feature flag #769

Merged
merged 5 commits into from
Oct 25, 2021
Merged

feat: add traceWithNft feature flag #769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
68458cc
feat: add aws-sdk to NFT ignore
eduardoboucas Oct 25, 2021
e5f300e
feat: add traceWithNft flag
eduardoboucas Oct 25, 2021
3039ab3
fix: make path Windows-friendly
eduardoboucas Oct 25, 2021
b86371a
fix: make ignore function compatible with Windows paths
eduardoboucas Oct 25, 2021
b373cab
refactor: adjust ignore glob
eduardoboucas Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/feature_flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const FLAGS: Record<string, boolean> = {
buildRustSource: Boolean(env.NETLIFY_EXPERIMENTAL_BUILD_RUST_SOURCE),
defaultEsModulesToEsbuild: Boolean(env.NETLIFY_EXPERIMENTAL_DEFAULT_ES_MODULES_TO_ESBUILD),
parseWithEsbuild: false,
traceWithNft: false,
}

type FeatureFlag = keyof typeof FLAGS
Expand Down
15 changes: 14 additions & 1 deletion src/runtimes/node/bundlers/nft/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { basename, dirname, join, normalize, resolve } from 'path'

import { nodeFileTrace } from '@vercel/nft'
import resolveDependency from '@vercel/nft/out/resolve-dependency'
import minimatch from 'minimatch'
import unixify from 'unixify'

import type { BundleFunction } from '..'
import type { FunctionConfig } from '../../../../config'
Expand All @@ -12,6 +14,9 @@ import { filterExcludedPaths, getPathsOfIncludedFiles } from '../../utils/includ

import { transpileMany } from './transpile'

// Paths that will be excluded from the tracing process.
const ignore = ['node_modules/aws-sdk/**']

interface NftCache {
analysisCache?: Map<string, { isESM: boolean; [key: string]: unknown }>
[key: string]: unknown
Expand Down Expand Up @@ -54,6 +59,13 @@ const bundle: BundleFunction = async ({
}
}

const ignoreFunction = (path: string) => {
const normalizedPath = unixify(path)
const shouldIgnore = ignore.some((expression) => minimatch(normalizedPath, expression))

return shouldIgnore
}

const traceFilesAndTranspile = async function ({
basePath,
config,
Expand All @@ -70,6 +82,7 @@ const traceFilesAndTranspile = async function ({
const { fileList: dependencyPaths } = await nodeFileTrace([mainFile], {
base: basePath,
cache,
ignore: ignoreFunction,
Copy link
Member

@Skn0tt Skn0tt Jun 27, 2023 •
edited

Choose a reason for hiding this comment

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

Just stumbled across this a millennia later. I see we're passing a custom ignoreFunction here that ends up doing glob matching. NFT docs say that you can pass an array of globs here, instead: https://github.com/vercel/nft/blob/314fd539335002273e77ba20d9832042af2a0bab/readme.md?plain=1#L166

What was the reasons against doing that? Is there a perf improvement / optimisation we could get from using an array here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea, to be honest. Maybe it had to do with cross-OS path normalisation (since I see unixify in there)? Or maybe the array option wasn't available at the time (or I simply didn't realise it was)?

Copy link
Member

Choose a reason for hiding this comment

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

It was available at the time - introduced in a commit named "first commit" :D vercel/nft@31d836e

I'll try replacing this function with an array in a different PR. If that doesn't break our tests (i'll check if we have one) then let's use the array instead.

readFile: async (path: string) => {
try {
const source = (await cachedReadFile(fsCache, path, 'utf8')) as string
Expand Down Expand Up @@ -136,7 +149,7 @@ const getSrcFiles: GetSrcFilesFunction = async function ({ basePath, config, mai
includedFiles,
includedFilesBasePath,
)
const { fileList: dependencyPaths } = await nodeFileTrace([mainFile], { base: basePath })
const { fileList: dependencyPaths } = await nodeFileTrace([mainFile], { base: basePath, ignore: ignoreFunction })
const normalizedDependencyPaths = dependencyPaths.map((path) => (basePath ? resolve(basePath, path) : resolve(path)))
const includedPaths = filterExcludedPaths([...normalizedDependencyPaths, ...includedFilePaths], excludedPaths)

Expand Down
8 changes: 7 additions & 1 deletion src/runtimes/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,17 @@ const getDefaultBundler = async ({
mainFile: string
featureFlags: FeatureFlags
}): Promise<NodeBundlerName> => {
const { defaultEsModulesToEsbuild, traceWithNft } = featureFlags

if (['.mjs', '.ts'].includes(extension)) {
return 'esbuild'
}

if (featureFlags.defaultEsModulesToEsbuild) {
if (traceWithNft) {
return 'nft'
}

if (defaultEsModulesToEsbuild) {
const isEsModule = await detectEsModule({ mainFile })

if (isEsModule) {
Expand Down
2 changes: 1 addition & 1 deletion tests/helpers/test_many.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const makeTestMany = (test, matrix) => {

const rateLimitedTestFn = getRateLimitedTestFunction(testFn)

rateLimitedTestFn(testTitle, assertions.bind(null, variation))
rateLimitedTestFn(testTitle, assertions.bind(null, variation), name)
netlify-team-account-1 marked this conversation as resolved.
Show resolved Hide resolved
})
}

Expand Down