Skip to content

Make default bundle paths in node-renderer and Rails consistent#399

Merged
justin808 merged 1 commit intomasterfrom
alexeyr/fix-default-bundle-path
Apr 27, 2024
Merged

Make default bundle paths in node-renderer and Rails consistent#399
justin808 merged 1 commit intomasterfrom
alexeyr/fix-default-bundle-path

Conversation

@alexeyr-ci
Copy link
Copy Markdown
Contributor

@alexeyr-ci alexeyr-ci commented Apr 26, 2024

See

dest_path = ENV["RENDERER_BUNDLE_PATH"].presence || Rails.root.join(".node-renderer-bundles").to_s

@alexeyr-ci alexeyr-ci force-pushed the alexeyr/fix-default-bundle-path branch from bc73283 to 45936e8 Compare April 26, 2024 14:20
Comment on lines -23 to +26
allWorkersRestartInterval: (env.CI && 2) || env.RENDERER_ALL_WORKERS_RESTART_INTERVAL || 10,
allWorkersRestartInterval: env.CI ? 2 : (env.RENDERER_ALL_WORKERS_RESTART_INTERVAL || 10),

// time in minutes between each worker restarting when restarting all workers
delayBetweenIndividualWorkerRestarts: (env.CI && 0.01) || 1,
delayBetweenIndividualWorkerRestarts: env.CI ? 0.01 : 1,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to improve readability.

delayBetweenIndividualWorkerRestarts: (env.CI && 0.01) || 1,
delayBetweenIndividualWorkerRestarts: env.CI ? 0.01 : 1,

// Uncomment and change value for testing the honeybadger API integration
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to uncomment.

@alexeyr-ci alexeyr-ci force-pushed the alexeyr/fix-default-bundle-path branch from 45936e8 to d2f83e9 Compare April 26, 2024 14:29

// time in minutes between restarting all workers
allWorkersRestartInterval: (env.CI && 2) || env.RENDERER_ALL_WORKERS_RESTART_INTERVAL || 10,
allWorkersRestartInterval: (env.CI ? 2 : env.RENDERER_ALL_WORKERS_RESTART_INTERVAL) || 10,
Copy link
Copy Markdown
Contributor Author

@alexeyr-ci alexeyr-ci Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(env.RENDERER_ALL_WORKERS_RESTART_INTERVAL || 10) would actually make more sense, but Prettier removes the parentheses and in my opinion having no parentheses at all is less readable.


// Find the .node-renderer-bundles folder if it exists, otherwise use /tmp
function defaultBundlePath() {
let currentDir = process.cwd();
Copy link
Copy Markdown
Contributor Author

@alexeyr-ci alexeyr-ci Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should start from __dirname instead? But I'd expect process.cwd() to usually be the Rails root in practice, so this makes the search quicker.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't use __dirname unless you want to depend on where node_modules is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it still usually be somewhere under Rails.root (in the cases we're on the same machine)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you get Rails.root in Node

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, node-renderer will either be under node_modules in the Rails root, or something like node-renderer-stand-alone in Popmenu, also under Rails root. In both cases going up from __dirname should work.


// Find the .node-renderer-bundles folder if it exists, otherwise use /tmp
function defaultBundlePath() {
let currentDir = process.cwd();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't use __dirname unless you want to depend on where node_modules is.

@justin808 justin808 merged commit 4b1db6c into master Apr 27, 2024
@justin808 justin808 deleted the alexeyr/fix-default-bundle-path branch April 27, 2024 07:11
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.

3 participants