-
Notifications
You must be signed in to change notification settings - Fork 893
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
Improper file extensions used in packaging #5839
Comments
Thanks for the report with the detailed explanation, @benmccann. We'll investigate this and check with feiyang or other engineers to see what we can do about this. |
@argzdev can you first see if you can reproduce this and assign back to me or feiyang to investigate? |
Hi @chliangGoogle, I was able to repro the issue with the given GitHub repro. I'll assign this case to you now, thanks! |
jacob-8/sveltefirets#4 may be caused by this issue so I'll keep an eye on this. |
Hi, just FYI, Feiyang has moved to another team so I can make any changes that are needed for this issue. Is messaging the only package affected? I'm asking because messaging is a little unusual, as it does not specifically provide "node" entry points in the "exports" field, for reasons given in the PR: #5646 As far as not having the mjs extension, I think what we did was we went with option 2 in https://nodejs.org/api/packages.html#determining-module-system where "Files ending in .js when the nearest parent package.json file contains a top-level "type" field with a value of "module"." We have a rollup plugin called If the other packages are working ok, then I think that method is ok and the problem with messaging might be that we don't have "node" entry points in "exports" which is due to the idb incompatibility mentioned in the PR. I think now that we have dropped IE support we may be able to upgrade our idb dependency version and maybe we can then add the Node ESM entry points for messaging, but that may be a moderate sized change as we use idb across multiple packages. If the other packages aren't working either then I guess we need to get rid of this workaround and just use mjs extensions, I believe Feiyang initially avoided this in order to not have to build an additional bundle but if it's necessary we can do it of course. |
Thanks so much for taking a look! Messaging is the only one I'm aware of being affected. I haven't checked any of the other packages. I don't use Firebase myself, but am a maintainer on SvelteKit and the only report we've gotten thus far is about messaging. Perhaps the issue lies with this particular directory that I'm getting an error message about:
No code resides in that directory and it's instead resolved from the I had tweaked the package locally awhile back and then started getting errors about IDB, so it may be required to upgrade it as you mentioned to get this fully working. I hadn't really looked into that yet and was just trying to get this first error out of the way 😄 |
So I can change the extension of However, I can fix it by changing
I'm a little bit of a newbie when it comes to usage of the I guess I'm just wondering if this will cause problems for any apps that expect only an ESM bundle when using |
That sounds probably okay and most likely better than the current situation. The main thing I'd be a little careful about is if you could end up with both a CJS and ESM version of the same file loaded and whether there would be any difficultly caused by that (e.g. two copies of any global variables). I'm not familiar with the Firebase codebase, so don't know if the change would be likely to cause any issue |
Hi @benmccann, Checking in on this issue. It looks like some work was done to progress this issue back in January of 2022. Is this still an problem for you? Thanks! |
Hey @benmccann. We need more information to resolve this issue but there hasn't been an update in 5 weekdays. I'm marking the issue as stale and if there are no new updates in the next 5 days I will close it automatically. If you have more information that will help us get to the bottom of this, just add a comment! |
https://publint.dev/@firebase/[email protected] is returning one suggestion for the packaging, but is no longer returning any errors, so this is likely fixed. Thanks! |
[REQUIRED] Describe your environment
[REQUIRED] Describe the problem
@Feiyang1 thanks for your efforts to improve packaging by adding
exports
to allpackage.json
files (#5646). Unfortunately, I didn't catch this when reviewing that PR, but I'm afraid there's still an issue.The file extensions are incompatible with Node. You must use
.mjs
for the ESM files (or set"type": "module"
and use.cjs
for the CJS files): https://nodejs.org/api/packages.html#determining-module-systemThis bug was originally filed in the SvelteKit project (sveltejs/kit#3083), but it's really an issue with the way Firebase is packaged. Vite will try to load Firebase directly using the Node APIs and will fail. It may work if you use a bundler like Webpack because the bundlers will transform the code to be valid before trying to run it, but Vite will try to run the code directly during SSR and will fail as a result.
Steps to reproduce:
Try to load
@firebase/messaging
from a Node app without bundling and it will failRun
npm install
followed bynpm run build
in this project: https://github.com/sacrosanctic/test-static-messagingYou will get the error:
@firebase/messaging/sw
is not CommonJS, but rather it's ESM. However, Node doesn't know that since there is neither"type": "module"
nor an.mjs
extension.Relevant Code:
The text was updated successfully, but these errors were encountered: