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

Merge prod.js into index.js #3808

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Jan 7, 2025

Brief summary

Merges prod.js and index.js.

Which issue is fixed?

No existing issue. I thought that having two different entry points into the code was not healthy and could lead to bugs.

In-depth Description

The new index.js has the commandline parameters and default values that prod.js had, as well the dev.js config functionality from the old index.js. The merge has a couple of logic changes:

  • --dev or -d flag to force NODE_ENV=development
  • Logging of the NODE_ENV value and of all of the arguments passed to the Server constructor, for better debuggability.

Consequently a couple of additional changes in other places were required:

  • Defining the required default environment variables explicitly in dockerfile (which is also good for readabilty)
  • Switcing from prod.js to index.js, and adding the --dev flag to the dev script in package.json

Note: in the new code, you need to run with NODE_ENV=development or --dev to force development mode, otherwise production is assumed.

How have you tested this?

  • Ran with --dev/-d
  • Built and run docker image with new ENV settings

@mikiher mikiher marked this pull request as ready for review January 7, 2025 16:46
@advplyr
Copy link
Owner

advplyr commented Jan 7, 2025

I'm thinking that we leave prod.js in for a release and add in the release notes a message that it will be removed soon.

The reason is because I'm sure there are some 3rd party install methods using prod.js. I found one https://github.com/YunoHost-Apps/audiobookshelf_ynh/blob/master/conf/systemd.service

I'm not sure if there are any other breaking changes we should make a notice of before requiring the change

@mikiher
Copy link
Contributor Author

mikiher commented Jan 8, 2025

Done.

@advplyr
Copy link
Owner

advplyr commented Jan 8, 2025

Thanks!

@advplyr advplyr merged commit 1649fb4 into advplyr:master Jan 8, 2025
5 checks passed
@mikiher mikiher deleted the merge-prod-js-index-js branch January 9, 2025 05:24
@Thovi98
Copy link

Thovi98 commented Jan 20, 2025

Thanks @advplyr for taking third party packages into account and for writing the breaking changes in the release note, it's verry helpful!

index.js Show resolved Hide resolved
@mikiher
Copy link
Contributor Author

mikiher commented Jan 21, 2025

I actually have a Synology NAS (though I'm not regularly using it as a docker container), and I was able to recreate this.

When using Synology Container Manager, it looks like when there's an existing container, its environment variables always remain the same as they were when the container was created, and an update of the image doesn't override the default set of environment variables of the existing container.

I believe that is not the expected behavior from a container manager, which should always apply the default environment variables defined in the image. So, again, I believe this is buggy behavior of the Synology Container Manager (it certainly doesn't seem like other Docker systems behave this way).

The workaround solution for this is to delete the container and rerun the docker run command

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.

4 participants