sveltejs / rollup-plugin-svelte

Compile Svelte components with Rollup
MIT License
505 stars 79 forks source link

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

Closed mationai closed 1 year ago

mationai commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

This was closed by #205