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

Any way to move "The following packages did not export their package.json" warning to onwarn? #181

Closed
mationai opened this issue Apr 7, 2021 · 15 comments

Comments

@mationai
Copy link

mationai commented Apr 7, 2021

Getting the "The following packages did not export their package.json" warning importing a package. I requested a PR for that package and owner closed it saying bundlers should use fs to read the package.json instead.

Regardless of the owner being right or not, is there any reason why this one has to be done with console.warn and not part of onwarn warnings? All warnings should be suppressible so users can suppress them once they see it and deemed not important.

@Conduitry
Copy link
Member

onwarn is currently being used exclusively for warnings that are coming from the Svelte compiler, and it would feel a bit weird to me to be using it for this one warning that comes from the bundler plugin. But I do see why that would be convenient. I'm torn.

PS the package owner is wrong

@Conduitry
Copy link
Member

If there were a nice way to use require.resolve that let us find the root of the package (as opposed to what they have as their pkg.main), then I wouldn't be totally averse to using fs.readFile to load the package.json and sidestep this whole issue. I think that'd be preferable to allowing this warning to be controlled. It's a reality that there just are going to be packages with export maps that don't export package.json and there's not really anything that can be done about that sometimes.

@mationai
Copy link
Author

mationai commented Apr 7, 2021

Thank you for your input. Looking at this from the outside, I don't see any reason why onwarn should only be used for plugin exclusive warnings. But since you are the maintainer of the repo, you have every right to keep that the way it is.

@caschbre
Copy link

caschbre commented Jul 2, 2021

I'm torn on this. I've run into the same thing where a package owner wouldn't change their package.json file because some other package is throwing warnings.

It's great that this warning is shown, but from a developer experience point-of-view, it gets very annoying after a while. I know the issue is there, but seeing the warning makes me thing I've got some issue in my code... or eventually leads me to ignore the console warnings (because they're always there) and then I miss a real issue.

@caschbre
Copy link

caschbre commented Jul 2, 2021

At this point I'm looking at something like https://github.com/ds300/patch-package to try and handle the patch locally. What "should" the package.json exports look like to remove the warning?

@mationai
Copy link
Author

mationai commented Jul 2, 2021

@caschbre
It's

	"exports": {
		".": "./dist/index.js",
		"./package.json": "./package.json"
	},

I even submitted a PR to the package owner, but it was rejected due to reason given in first comment.

I am not going to say who is right or not since I'm just thankful for both party's time and effort in these OSS libraries. However, I just don't see why onwarn must be used only for plugin exclusive warnings and at the same time, not having something like some-other-better-named-onwarn for non-plugin exclusive warnings that users can suppress.

@odeniso
Copy link

odeniso commented Sep 1, 2021

I've tried making the change in a package here, and the MR was closed as well.

https://github.com/sveltejs/rollup-plugin-svelte#pkgsvelte
What about not showing the error at all? The "svelte" property is an opt-in feature for svelte packages only. Most other packages need not care about it, and therefore need not export their package.json.


(If explicitly exporting the package.json is actually very common practice, a reference would be very helpful in convincing package maintainers.)

Warning: Introducing the "exports" field prevents consumers of a package from using any entry points that are not defined, including the package.json (e.g. require('your-package/package.json'). This will likely be a breaking change.
https://nodejs.org/api/packages.html#packages_package_entry_points


I find it difficult to accept that the vast majority of packages would easily accommodate this request. It is only a question of time before more packages use the relatively new "exports" field along with the automatic encapsulation that comes with it.

I do understand that removing this error places a burden on svelte component packages. They will need to consult documentation and be aware of best practices. Is there any way we can scan a package's exports for svelte components, before showing this message?

@lukeed
Copy link
Member

lukeed commented Sep 1, 2021

What about not showing the error at all?

It's a warning, not an error.

I find it difficult to accept that the vast majority of packages would easily accommodate this request

There is lots of incomplete and/or misinformation regarding ESM and exports at large. This "rollout"/cleanup-effort is still underway.

Is there any way we can scan a package's exports for svelte components, before showing this message?

That's exactly what's being done & what the warning is about: "Hey, we weren't allowed to look at X,Y,Z package.json files, which means we're unable to determine if we're missing any "svelte" fields from these packages"

@odeniso
Copy link

odeniso commented Sep 1, 2021

@lukeed Regarding the last point, I meant whether the plugin can scan what is actually being exported by the package (without looking at the package.json), and determine if svelte components components are included. If so, this warning could still be surfaced, but only in those cases.

i.e "Hey, I can see that some svelte components are included, but this package is not letting me get to their uncompiled source code"

@lukeed
Copy link
Member

lukeed commented Sep 1, 2021

So to @Conduitry's earlier point, the quickest way to achieve that would be require.resolve("foobar/package.json"), which is also rejected because it triggers the "can I access this?" guard.

Since that's off the table, have to remain with require.resolve("foobar") so that you end up at some final JS destination file. Problem here is that it could be "foobar/lib/index.js", "foobar/index.js", "foobar/build/browser/esm/index.js", ...etc.

Have to use require.resolve here because there's unfortunately little assumption you can make about where the module resides. Very easy for it not to be same node_modules as the Rollup plugin. The resolver needs to start from wherever the importer's current location is, which require.resolve allows.

Wherever/whatever the output file path is determined to be, this plugin would then have to recursively scale that file's directories until it finds the root package.json file. That's another set of assumptions we have to take on in order to determine the module root.

All that is to say :: it's not impossible, but it's a truck load of filesystem operations for a simple task, which may prove to be tricky to get right in all cases anyway. It's exactly the kinda guesswork that "exports" was meant to solve/replace.

@caschbre
Copy link

caschbre commented Sep 5, 2021

I'm ok with rollup-plugin-svelte not trying to do more to handle other packages not exporting their package.json. I guess all I would be looking for is some configuration (e.g. in rollup.config.js) where we can list the libraries to skip the warning for. Essentially a way to say "yep, I've seen the warning. Now stop showing it to me for this library" :-)

@tborychowski
Copy link

tborychowski commented Sep 7, 2022

Surprisingly I only started to see it now, but it's very annoying.

I don't care about this warning.
Package works.
The export code someone suggested above is not something that all packages will have, so why do we have to see an unhideable warning about it?

Moreover, why would anyone export package.json from package.json? (serious question)
If you can read package.json to read what it exports, you can read package.json, can't you?
(or am I missing something here?)

Lastly, warnings are usually there for devs to know that they should fix something.
I cannot fix other people's packages!

@tborychowski
Copy link

As expected - 3rd party libraries do not give a damn about adding weird constructs to their code, just to satisfy your warning logger, so can we please be done with it and drop this stupid warning already?

@benmccann
Copy link
Member

benmccann commented Nov 3, 2022

We will probably remove resolution of pkg.svelte which will end up solving this. However, we'll need to introduce a warning about the upcoming behavior change for a period of time first for packages where that would change how the package is resolved.

@Rich-Harris
Copy link
Member

This was closed by #205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants