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

"Module" before "main" is default in Node Webpack builds, now generates an (obnoxious) warning #1612

Closed
arpowers opened this issue Mar 18, 2019 · 14 comments

Comments

@arpowers
Copy link

arpowers commented Mar 18, 2019

In reference to this conversation:
#1536 (comment)

Issue at Hand: There is a problem with assuming the "module" field in package.json is for the browser. Webpack (and everyone else) is assuming 'module' is the go-to package for node!

I would suggest making the Browserify (or similar) use-case the "special-case" because remember: ANY SSR use of Firebase is going to get this warning!

Other packages definitely throw nasty errors in Node if "main" is processed before "module". I can tell you because I get 4 or 5 issues just on my setup here.

Solutions:

Remember Transpilation is Really Easy
You are trying to make something too easy. Transpilation from esm to cjs and so on is easy with transpilation. I do it all the time. Why don't you just recommend this to people who need it? The solution you are providing is NOT easy to remedy because of the "rock in a hard place" nature of "broken if you do, broken if you don't."

Thread
I don't know if you've tried this:
webpack/webpack#4674

A Flag to Disable the Warning?
The 'module' package runs fine in Node actually; its the massive warning thats the problem (in my case).

Use Case:
Vue JS - Server-Side Rendered App

  • Operating System version: Latest Mac OSX
  • Browser version: Latest Chrome
  • Firebase SDK version: Latest
  • Firebase Product: Auth, Firestore, more...

@Feiyang1

@Feiyang1
Copy link
Member

Feiyang1 commented Mar 20, 2019

Are you doing SSR or making a bundle targeting NodeJs? If you are doing SSR and you know you don't need to run any firebase code(e.g. firestore), can you just ignore it? We have seen people who want to target NodeJs, but included the browser build incorrectly wondering why it doesn't work. This message is very useful for them for debugging.

As to which field should be used for what build, I think it's bit of a mess for packages that provide different node and browser builds, and there is no consensus at the moment.

Can you please clarify who everyone else you are referring to? I'm not super up to date on it, but I know rollup (rollup-node-resolve) doesn't assume it. Also unpkg and polymer-cli all look at module field first for loading module files natively in browser.

BTW, I have looked at webpack/webpack#4674 (comment), but it only works for webpack( e.g. rollup doesn't understand this syntax).

So I think there is no perfect solution here (maybe we can provide separate packages for browser and node, but it will have a large impact and needs more discussion).
For now, I think this warning message solves a big pain point for Nodejs bundles, so I lean toward keeping it, though it is kind of annoying for some use cases (e.g. SSR without running firebase code).

@arpowers
Copy link
Author

arpowers commented Mar 21, 2019

@Feiyang1

We are using 'vue-server-renderer' to create a bundle that is then used by the Node server to render the page. Calls to Firestore are made at runtime, to do prefetches and so on...

I've created a video showing my current experience and the warnings i'm getting when taking your Webpack suggestion (vs the Firebase warning when I don't)

https://drive.google.com/file/d/1Vr_D-bxROTUWz2Dagxe1EyD8gk4uvcXY/view

Screen Shot 2019-03-21 at 10 57 47 AM

Also, off-topic, but I'm developing Firebase into an open-source serverless CMS and would appreciate a call with some engineers to work through some of the issues we're having. Not sure who to talk with...

@Feiyang1
Copy link
Member

@samtstern What support channel can @arpowers contact?

@samtstern
Copy link
Contributor

@arpowers that sounds cool! However we are a small team and can't offer calls with individual developers, we'd be on the phone all day and get nothing done!

I'd suggest posting about your ideas on the firebase-talk mailing list once you have something to show:
https://groups.google.com/forum/#!forum/firebase-talk

You may also want to talk to the developers of one of these Firebase CMS services:
https://flamelink.io/
https://www.pushtable.com/

@arpowers
Copy link
Author

@samtstern this product could bring thousands of new Firebase customers (we have created Frameworks used by 100k+ sites already), but everytime i run into a problem i get dead ended. Support people don't seem to know anything about solutions to even moderately complicated problems.

@samtstern
Copy link
Contributor

@arpowers if you want to email me a list of questions about Firestore, I am happy to answer them or try and find answers.

My email address is on my GitHub profile.

@arpowers
Copy link
Author

arpowers commented Apr 18, 2019

@Feiyang1 any updates on this?

Are there alternative ways of importing Firebase components aside from 'firebase/app' etc... ?

I'm making a public CLI which is why this is important.

@Feiyang1
Copy link
Member

Feiyang1 commented Apr 18, 2019

Are you facing any new issue with the CLI, or just the warning message? I'm asking because you mentioned SSR was the problem, just want to clarify the CLI use case here. Thanks.

@mhofman
Copy link

mhofman commented May 5, 2019

BTW, I have looked at webpack/webpack#4674 (comment), but it only works for webpack( e.g. rollup doesn't understand this syntax).

My reading of rollup/rollup-plugin-node-resolve#183 is that the syntax should work with rollup as well.

@evdama
Copy link

evdama commented Jan 16, 2020

Is there any new best-practice or otherwise progress on the SSR subject? Similiar to Angular, Vue SSR and React users I'm having import issues Sapper (a SSR toolkit for Svelte vs3). The issues are as mentioned here codediodeio/sveltefire#4 here #1797 here #1754 (comment) and here #1455 (comment)

I'm looking forward for node v13 being available on firebase so we can be using the ESM format there as well, but then I guess that might take a while and we need to find a solution that works for SSR until that happens :)

@fkrauthan
Copy link

fkrauthan commented Oct 8, 2022

Hey is there any news? I get the issue using pnpm with create react app (webpack). It looks like the issue is that it tries to pull in index.esm2017.js which contains a const json = require(defaultsJsonPath); line that triggers the warning in webpack: Critical dependency: the request of a dependency is an expression. By default, it's a warning but create react app converts that into an error for production builds, therefore, my builds are failing.

@ChromeQ
Copy link

ChromeQ commented Oct 9, 2022

Same problem as @fkrauthan above. For me seems to be only happening with updating firebase from 9.10.0 to 9.11.0.

@hsubox76
Copy link
Contributor

That's a bug and will be fixed in the release this week. See #6660 I don't think it's related to this older issue.

@hsubox76
Copy link
Contributor

hsubox76 commented Aug 7, 2023

This warning message is no longer in the codebase at this time, closing the issue.

@hsubox76 hsubox76 closed this as completed Aug 7, 2023
@firebase firebase locked and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants