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

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

Closed
1 of 2 tasks
gregjoeval opened this issue Aug 4, 2020 · 13 comments · Fixed by #3559
Closed
1 of 2 tasks
Labels
external issue The root cause is with an external component that needs a fix or workaround

Comments

@gregjoeval
Copy link

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

Is this a feature or a bug?

  • Feature
  • 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.

  • Tool: @microsoft/api-extractor
  • Tool Version: ^7.9.2
  • Node Version: v13.9.0
    • Is this a LTS version? no
    • Have you tested on a LTS version? no
  • OS: Windows 10
@gregjoeval
Copy link
Author

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

@vidartf
Copy link
Contributor

vidartf commented Aug 24, 2020

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. mui/material-ui#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
Copy link
Collaborator

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?)

@octogonz octogonz added the external issue The root cause is with an external component that needs a fix or workaround label Aug 24, 2020
@gregjoeval
Copy link
Author

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
Copy link

eps1lon commented Aug 24, 2020

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
Copy link
Collaborator

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
Copy link
Contributor

vidartf commented Aug 25, 2020

Note that for NPM it says

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

@vidartf
Copy link
Contributor

vidartf commented Aug 25, 2020

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:

  • For this case, it will also throw this exception if the package.json is in a dependency (i.e. for our example case not just for materialui, but for any project that depends on it directly or indirectly).
  • The exception in question is uncaught. It should at least be caught and turned into a warning that is possible to ignore, instead of dumping a full trace to the output for older versions of Node (and for newer/future versions of Node, unhandled rejections in promises will cause the app to fall over, it warns). As such, we might not need to make the call about whether it is safe to ignore or not, but leave it to the user.

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
Copy link

eps1lon commented Aug 25, 2020

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
Copy link

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
Copy link

jc-1234 commented Jul 19, 2021

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
Copy link

This is also a problem with maplibre-gl: https://unpkg.com/browse/[email protected]/dist/package.json

@matthewborgman
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external issue The root cause is with an external component that needs a fix or workaround
Projects
None yet
8 participants