do not copy newtonsoft.json to Revit/Nodes folder

DynamoDS / DynamoRevit

Dynamo Libraries for Revit

https://dynamobim.org

338 stars 188 forks source link

do not copy newtonsoft.json to Revit/Nodes folder #2885

Closed mjkkirschner closed 1 year ago

mjkkirschner commented 1 year ago

Purpose

unify references to not include version number do not copy to nodes folder

⚠️ note If you are testing this change locally I had to manually delete the old copy of newtonsoft.json from the Revit.Nodes folder - a rebuild would not delete it for some reason.

Declarations

Check these if you believe they are true

[ ] The code base is in a better state after this PR
[ ] Is documented according to the standards
[ ] The level of testing this PR includes is appropriate
[ ] User facing strings, if any, are extracted into *.resx files
[ ] Snapshot of UI changes, if any.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

QilongTang commented 1 year ago

@wangyangshi if there will be more hotfix for past D4R releases, you may want to consider hotfix as well.

wangyangshi commented 1 year ago

will this fix the steel connection issue in dynamo 2.17? I am not very clear why we need to change it in earlier D4R releases, because in before versions, it has no issues. if my understanding not right, please correct me, thank you

QilongTang commented 1 year ago

will this fix the steel connection issue in dynamo 2.17? I am not very clear why we need to change it in earlier D4R releases, because in before versions, it has no issues. if my understanding not right, please correct me, thank you

I would say in short this implementation will easily cause dependency loading conflicts. This could cause problems whenever there is a version mismatch of NewtonSoft.Json dll. The bug reported by AS team is just one instance where other users could run into but did not report it. Since this is a compile-time preference, as @mjkkirschner mentioned, let's not copy this dll to nodes folder, because NewtonSoft.Json is almost guaranteed to be loaded by Dynamo or Revit already, there is no need to load another hard copy of it.

Hotfix is probably not a must do, just a suggestion to include if in the future you are going to do other hotfixs. But this should at least fix it for Global Launch.

wangyangshi commented 1 year ago

OK, understand it, thank you Aaron for the explanation!

wangyangshi commented 1 year ago

after testing, this fixed the regression issue, can it be merged now?

QilongTang commented 1 year ago

after testing, this fixed the regression issue, can it be merged now?

Yeah if you approve the PR. Feel free