Skip to content

Revisiting PG and Create React App #1822

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

Closed
Downchuck opened this issue Jan 25, 2019 · 8 comments
Closed

Revisiting PG and Create React App #1822

Downchuck opened this issue Jan 25, 2019 · 8 comments

Comments

@Downchuck
Copy link

While there have been several notes about Webpack, and how node-postgres does not run in the browser, there is it seems only one issue in one code section that is causing grief.

If I go ahead into pg/lib/connection-parameters.js and comment out the dns call, then I no longer have an ache in my heart, and I'm able to compile a server-side app for some client-side use cases, and can reasonably mock the postgres library and support the pg.Pool construct.

I'll continue looking for a work-around (other than commenting out five lines of code), but at present it seems that both create-react-app and the node-postgres projects are at an impasse on this corner-case.

To some extent, this could be read as: "make pg mockable on the client-side", "make the dns requirement optional" or "make this compatible with create-react-app". To those specifically and directly using webpack, they have options for simply mocking the dns requirement.

Again, I understand this is a corner case, but in working with a rather large project, this has been the only issue I've been unable to work around.

@Downchuck
Copy link
Author

Example of connect-parameters.js which create-react-app will compile:

  try {
  var dns = require('dns')
  dns.lookup(this.host, function (err, address) {
    if (err) return cb(err, null)
    params.push('hostaddr=' + quoteParamValue(address))
    return cb(null, params.join(' '))
  })
  } catch(e) {
    return cb(null, params.join(' '))
  }

@serv
Copy link

serv commented Jan 28, 2019

I don't think node-postgres should change to support its use on the browser. It is reasonable that postgres db is connected from server side for security reasons. Given that majority of use cases for the package is only on the server side, I don't support the package changing to suit the usecase on the browser.

@kibertoad
Copy link
Contributor

Webpack doesn't necessarily mean browser per se. I think I've seen quite a bit of usage of webpack+persistence layer from Electron apps.

@sehrope
Copy link
Contributor

sehrope commented Jan 28, 2019

What's the goal of this? Besides the "dns" module there's a couple other core NodeJS modules that wouldn't be available in a browser runtime such as "net" (for creating TCP sockets). If you can't actually open a TCP socket or resolve a DNS hostname, what could you use this for?

If you just want to get webpack to skip over the missing deps you can use the Webpack IgnorePlugin. There's a previous issue in this repo with some more detail: #1187

@Downchuck
Copy link
Author

Downchuck commented Jan 28, 2019

This is a shortcoming of Create React App (CRA) which eschews webpack customization. That stated, of the 750 dependencies in the server-side of this application, and every line of code in node-postgres, the only issue I have hit has been with the connect-parameters.js file.

I am working with code from a client-server project and presenting several modules as isolated items in the browser for development/testing. There is quite a bit of logic on the server-side which runs just fine on the client side for isolating behaviors for development.

It's simply a pain point that I'm facing as I'm isolating code paths for development and maintenance of a large application (approx. 50k LOC) written over the last couple years.

Presently, I manually change the connect-parameters.js file every time I run an npm install on a machine. While I've every opportunity to "eject", I am trying not to add more customization, or fork Create React App.

Notably, other modules, such as redis, work just fine, and items like redis-mock slip right in, by mocking Redis.createClient for instance. In Postgres, one might assume that one could mock pg.Pool -- but because there are no try guards around connection-parameters require DNS, it falls apart in the default configuration.

Isolation of components:
https://facebook.github.io/create-react-app/docs/developing-components-in-isolation

Ejecting from Create React App:
https://facebook.github.io/create-react-app/docs/available-scripts#npm-run-eject

Alternatives to ejecting:
https://facebook.github.io/create-react-app/docs/alternatives-to-ejecting

@Downchuck
Copy link
Author

I'm going to try some more workarounds including a try catch around require pg with a fallback to the Pool module as an interface (this project has a bit of typescript as well).

The code around how pg.Pool works is a little odd.

@serv - this is not intended to literally run Postgres in the browser. It's to use the interfaces of pg.Pool such as query, but intercept the calls, while keeping present code mostly as-is, so that coverage in testing is reasonably formed to the code paths actually run in production.

@kibertoad - I suspect electron apps have access to dns.

@sehrope - I am trying to avoid forking react-scripts and creating yet another custom managed repo.

I am going to try again today with some try catch blocks in my postgres requires and other such methods to work around this.

At present I have spent a few hours doing everything I can to avoid a custom node-postgres install and/or an ejected create react app/react-scripts fork. This includes trying sinon and rewiremock and other strange bends.

In my mind, this is a frustrating choice by the Create React Apps project not to have stubs for this nodejs module. Notably I've not has issues with fs or http.

-Charles

@Downchuck
Copy link
Author

I've submitted a PR over to create react app (CRA) -- it may be fixed with the one liner of adding an empty mock for DNS. It helps that this issue is here, to let them know why the PR is valuable.

I believe if they accept the PR, than any folk that attempt to use node-postgres with CRA will simply need to add to their code once: require('dns').lookup = cb => cb("no dns");

@Downchuck
Copy link
Author

Fixed in create-react-app using dns mock webpack setting.

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

No branches or pull requests

4 participants