Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: avoid packageJson without name in resolveLibCssFilename #19324

Merged
merged 5 commits into from
Feb 4, 2025

Conversation

Jinjiang
Copy link
Contributor

Description

Fix the issue when there is no package name in package.json.

which causes errors like below:

[vite:css-post] Cannot read properties of undefined (reading '0')
[vite:css-post] Cannot read properties of undefined (reading '0')
    at getPkgName (/.../node_modules/vite/dist/node/chunks/dep-M1IYMR16.js:10488:14)
    at resolveLibCssFilename (/.../node_modules/vite/dist/node/chunks/dep-M1IYMR16.js:50127:30)

@patak-dev
Copy link
Member

Why does the package.json doesn't have a name? It is required:

A package.json file must contain "name" and "version" fields.

Maybe we should improve the error instead saying that the package.json is malformed?

@silverwind
Copy link

silverwind commented Jan 31, 2025

name and version may be required in the docs, but as far as the npm client is concerned {} is a valid package.json, so I suggest to treat is gracefully.

I think those requirements are only enforced for published packages.

patak-dev
patak-dev previously approved these changes Jan 31, 2025
@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 31, 2025
Copy link
Member

Choose a reason for hiding this comment

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

I think this part needs to be fixed as well.

const packageJson = findNearestPackageData(root, packageCache)?.data
const name =
libOptions.fileName ||
(packageJson && typeof libOptions.entry === 'string'
? getPkgName(packageJson.name)
: entryName)
if (!name)
throw new Error(
'Name in package.json is required if option "build.lib.fileName" is not provided.',
)

Copy link
Contributor Author

@Jinjiang Jinjiang Feb 4, 2025

Choose a reason for hiding this comment

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

@sapphi-red Just confirm: I saw several test cases particular on packages/noname. Do you think it should be updated to packages/name as well? Thanks.

https://github.com/vitejs/vite/blob/main/packages/vite/src/node/__tests__/build.spec.ts

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be packages/noname as-is, but the resolved name should be updated so that we test the fallback behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test case updated. I also added an additional packages/vite/src/node/__tests__/packages/package.json to catch the resolver for noname. Please take a look. 🙏

Comment on lines 3465 to 3467
const packageJson = findNearestPackageData(root, packageCache)?.data
const name = packageJson ? getPkgName(packageJson.name) : undefined
const name =
packageJson && packageJson.name ? getPkgName(packageJson.name) : undefined
Copy link
Member

Choose a reason for hiding this comment

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

We have findNearestMainPackageData function that skips the package.json without name field. I think swapping findNearestPackageData with it matches the users expectations.

// Finds the nearest package.json with a `name` field
export function findNearestMainPackageData(

@sapphi-red
Copy link
Member

sapphi-red commented Feb 3, 2025

Sometimes there're nested package.jsons inside a package. This is often used to map require('pkg/dir') to somewhere else (e.g. pkg/dir/dir.js). We don't need this anymore as most bundlers support exports field though.
https://cdn.jsdelivr.net/npm/primevue@4.2.5/button/package.json
The other case I saw was placing package.json only with { "type": "commonjs" } in a directory so that all files in that directory is treated as CJS.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks!

@sapphi-red sapphi-red changed the title fix: avoid packageJson without name in resolveLibCssFilename fix: avoid packageJson without name in resolveLibCssFilename Feb 4, 2025
@patak-dev patak-dev merged commit f183bdf into vitejs:main Feb 4, 2025
16 checks passed
@Jinjiang Jinjiang deleted the patch-1 branch February 4, 2025 09:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants