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/resources-release #514

Merged
merged 3 commits into from
Apr 22, 2024
Merged

fix/resources-release #514

merged 3 commits into from
Apr 22, 2024

Conversation

PaulDalek
Copy link
Contributor

Added fixes and corrections to releasing pool and resources, closing servers and clearing intervals.

@PaulDalek PaulDalek requested a review from jszuminski April 11, 2024 13:21
@PaulDalek PaulDalek self-assigned this Apr 11, 2024
Copy link
Contributor

@jszuminski jszuminski left a comment

Choose a reason for hiding this comment

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

I've noticed that when running npm run node-tests I get the following error:

[Fail] Node module from file: options_stringified.json, took: 133ms.
Mon Apr 22 2024 10:21:12 GMT+0200 [error] - Cannot create property 'chart' on string '{"title":{"text":"Stringified options"},"xAxis":{"categories":["Jan","Feb","Mar","Apr","May","Jun","Jul","Aug","Sep","Oct","Nov","Dec"]},"series":[{"type":"column","data":[1,3,2,4]},{"type":"column","data":[5,3,4,2]}]}' 
 TypeError: Cannot create property 'chart' on string '{"title":{"text":"Stringified options"},"xAxis":{"categories":["Jan","Feb","Mar","Apr","May","Jun","Jul","Aug","Sep","Oct","Nov","Dec"]},"series":[{"type":"column","data":[1,3,2,4]},{"type":"column","data":[5,3,4,2]}]}'
    at doExport (file:///Users/jakubszuminski/Documents/Coding/Highcharts/exporting/node-export-server/lib/chart.js:360:21)
    at Object.startExport (file:///Users/jakubszuminski/Documents/Coding/Highcharts/exporting/node-export-server/lib/chart.js:108:11)
    at file:///Users/jakubszuminski/Documents/Coding/Highcharts/exporting/node-export-server/tests/node/node_test_runner.js:95:18
    at new Promise (<anonymous>)
    at file:///Users/jakubszuminski/Documents/Coding/Highcharts/exporting/node-export-server/tests/node/node_test_runner.js:72:13
    at Array.map (<anonymous>)
    at file:///Users/jakubszuminski/Documents/Coding/Highcharts/exporting/node-export-server/tests/node/node_test_runner.js:70:10

but I've checked and it appears on master as well so this is not a part of this pull request.

Copy link
Contributor

@jszuminski jszuminski left a comment

Choose a reason for hiding this comment

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

Looking great!

@PaulDalek
Copy link
Contributor Author

@jszuminski Thanks for the reveiw and noticing the problem with the test. Will investigate it separately and fix it.

@PaulDalek PaulDalek merged commit 693faaf into master Apr 22, 2024
2 checks passed
@PaulDalek PaulDalek deleted the fix/resources-release branch April 22, 2024 09:54
@mikemeerschaert
Copy link

FYI this PR appears to remove the killPool function. I'm not sure if this was intentional or is supposed to be replaced by the shutdownCleanUp function, but if that is the case, it means you can't clean up the highcharts pool while still executing code in a node module since it causes the whole process to exit.

@PaulDalek
Copy link
Contributor Author

@mikemeerschaert Thanks for noticing that! The initPool and killPool are available again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants