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

fix (starter): remove --no-warnings flag #435

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

kevin-dp
Copy link
Contributor

@kevin-dp kevin-dp commented Sep 18, 2023

Removes the --no-warnings flag and sidesteps the warning by not importing JSON file directly (which is experimental) but instead reading the file and parsing it as JSON.

Tested with Docker, could reproduce the issue and confirm that it is fixed by this PR.

Fixes #422

@linear
Copy link

linear bot commented Sep 18, 2023

VAX-998 Starter: remove --no-warnings flag

The --no-warnings flag seems to be causing some issues depending on the implementation of env.

Removing the --no-warnings flag solves this issue but we should double check the output to see that it does not become to verbose.

See:

#422

@kevin-dp kevin-dp changed the title fix (starter): Remove --no-warnings flag fix (starter): remove --no-warnings flag Sep 18, 2023
@thruflo
Copy link
Contributor

thruflo commented Sep 18, 2023

Can I just sanity check that you've tested an e2e starter flow on ubuntu?

@alco
Copy link
Member

alco commented Sep 18, 2023

I've googled to see if there's a way to apply the "no-warnings" setting once Node is launched. There appear to be two working solutions:

@kevin-dp
Copy link
Contributor Author

kevin-dp commented Sep 18, 2023

Can I just sanity check that you've tested an e2e starter flow on ubuntu?

I have:

  • Started a Docker container using node image: docker run -it node /bin/bash
  • Inside the container i cloned the github repo, went to the starter template, built it, linked it
  • Then still from within the container, i tested create-electric-app foo, runs without problems.

That should confirm that the problem is gone.
I have not ran the example app in the Docker container because i don't know how to achieve that (as it would essentially have to run in the browser).

Note that the node image i used uses debian:12.
But i tested that that version also has the same problem as ubuntu, which it does.

EDIT: I'm also thinking of writing a test to check that the starter template works. At least up to the point where the project structure is initialized. But should probably be a separate PR.

@kevin-dp
Copy link
Contributor Author

kevin-dp commented Sep 18, 2023

I've googled to see if there's a way to apply the "no-warnings" setting once Node is launched. There appear to be two working solutions:

* call `process.removeAllListeners('warning')` as explained here -- [Node warnings bubble up and in bin scripts can't be disabled  nodejs/node#32876 (comment)](https://github.com/nodejs/node/issues/32876#issuecomment-616709931) (found via [Cannot disable warnings when node is launched via a shell script. nodejs/node#10802 (comment)](https://github.com/nodejs/node/issues/10802#issuecomment-616728458))

* spawn a child Node process (letting the OS resolve the path to the executable) and pass the `--no-warnings` flag to it -- https://stackoverflow.com/a/56095929

Yes, might be useful in the future if we really do need to pass some flags.
However, in this case, i think the current solution (read file then parse it as JSON) is perfectly fine.

@thruflo
Copy link
Contributor

thruflo commented Sep 18, 2023

LGTM 👍

@kevin-dp kevin-dp merged commit bec4399 into main Sep 18, 2023
1 check passed
@kevin-dp kevin-dp deleted the kevindp/vax-998-remove-node-flag branch September 18, 2023 12:23
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 this pull request may close these issues.

[VAX-1003] create-electric-app with asdf node
3 participants