microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.82k stars 592 forks source link

[api-extractor] Partial package.json files (used by bundlers for tree shaking) prevent .d.ts rollup #2070

Closed gregjoeval closed 2 years ago

gregjoeval commented 4 years ago

Please prefix the issue title with the project name i.e. [rush], [api-extractor] etc.

Is this a feature or a bug?

Please describe the actual behavior. Valid partial package.json files of a dependency result in the following error message after executing api-extractor run

Error reading "F:\Projects\GitHub\package-library\packages\material-ui-adjunct\node_modules\@material-ui\core\Grid\package.json": The required field "name" was not found

If the issue is a bug, how can we reproduce it? Please provide detailed steps and include a GitHub branch if applicable. Your issue will get resolved faster if you can make it easy to investigate. In a project that uses a package with partial package.json files (for example: Material-UI), prepare the project for api-extractor with .d.ts rollup, execute api-extractor run.

What is the expected behavior? Successfully execute api-extractor run with .d.ts rollup enabled while using packages with partial package.json files

If this is a bug, please provide the tool version, Node.js version, and OS.

gregjoeval commented 4 years ago

I can provide a more in depth example in one of my own repos if necessary

vidartf commented 4 years ago

Put in a different way: Certain third-party dependencies might put invalid package.json files at random locations in their source tree and that might work perfectly fine for them (e.g. https://github.com/mui-org/material-ui/issues/15715 to add context to the OP example). The question then becomes, is there a way for AE to gracefully handle such files, or is this a strict incompatibility?

octogonz commented 4 years ago

material-ui's package.json files violate at least two specifications for this file:

The question then becomes, is there a way for AE to gracefully handle such files, or is this a strict incompatibility?

If we make a special accommodation for material-ui, what would it be?

In that thread, @eps1lon wrote:

These are not standalone packages. The package.json is just there to assist bundlers in finding the module builds. Is there a documentation for required name fields on npm? Otherwise this sounds like the api-extractor tool should just ignore those packages.

So the questions would be:

  1. Will API Extractor produce correct output if we simply ignore these invalid files?
  2. API Extractor's main role is to protect against mistakes for professionally shipped software, so generally it does not have a philosophy of silently ignoring errors. How should we determine whether a file is safe to ignore? (Missing name field?)
gregjoeval commented 4 years ago

Are there other packages that utilize a similar method of bundle splitting? If this is unique to material-ui maybe it would be better to work with them to figure out a way to add those properties to their partial package.json files

eps1lon commented 4 years ago

Are there other packages that utilize a similar method of bundle splitting? If this is unique to material-ui maybe it would be better to work with them to figure out a way to add those properties to their partial package.json files

Check out this twitter thread: https://twitter.com/mjackson/status/1295726938833657856

It lists a couple of packages that use this method which just leverages the node resolution algorithm.

CommonJS: name and version are required fields: http://wiki.commonjs.org/wiki/Packages/1.0

This defines the top-level package.json. It does not specify how nested package.json should look.

I think the same applies to npm though they don't explicitly mention top-level package.json

octogonz commented 4 years ago

This defines the top-level package.json. It does not specify how nested package.json should look.

How would we detect a "top-level" package.json? Node.js module resolution works by crawling directory trees. It doesn't really distinguish installed NPM packages versus local projects.

vidartf commented 4 years ago

Note that for NPM it says

If you don’t plan to publish your package, the name and version fields are optional.

vidartf commented 4 years ago

So the questions would be:

1. Will API Extractor produce correct output if we simply ignore these invalid files?

2. API Extractor's main role is to protect against mistakes for professionally shipped software, so generally it does not have a philosophy of silently ignoring errors.  How should we determine whether a file is safe to ignore?  (Missing `name` field?)

For the record, I do believe these are the questions that need answering.

Re pt 2: I agree with this philosophy, but make the following notes:

Re pt1: That depend on whether or not AE uses the name/version fields for anything in the code, or if it just checks out of caution. Normally NPM will prevent you from publishing if the package.json files does not include a name/version field, so the check might be redundant if AE doesn't actually use the fields for anything.

eps1lon commented 4 years ago

How would we detect a "top-level" package.json?

I don't know since I'm not using api-extractor nor involved with its implementation.

I'm just pointing out that this pattern is not violating any standard, is used beyond Material-UI and respected by webpack (and I believe rollup and possible every other bundler).

If you decide to keep throwing when encountering this pattern then it'd be nice if you could advise on an alternate implementation that does not break app bundles produced by webpack or rollup.

sunnysingh commented 3 years ago

Hi, any updates on this issue? We are currently unable to use API Extractor since it doesn't work with Material-UI. We could use something like npm.im/patch-package but it also seems to have issues with Rush, plus an official fix would be more ideal than a monkeypatch.

jc-1234 commented 3 years ago

Any updates? This is preventing us from upgrading to PrimeReact v6.5 because they have package.json files in subfolders of their components that are missing name property.

sebisteiner commented 2 years ago

This is also a problem with maplibre-gl: https://unpkg.com/browse/maplibre-gl@2.1.9/dist/package.json

matthewborgman commented 2 years ago

Is there an intent to try to fix this? If not, this seems like the issue should be called out somewhere in the documentation.

Like @sunnysingh, my project is unable to use this tool, as much as we'd like, due to an "intermediary" TailwindCSS package that does not contain the name field.