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

feat: add support for exports not named the same as the class #25

Merged
merged 2 commits into from
Jun 8, 2020

Conversation

Julusian
Copy link
Collaborator

@Julusian Julusian commented Mar 1, 2020

Raised from nrkno/sofie-atem-connection#74

In some cases the name of the export may not match the name of the class. Often when the code is minified, or re-exported over multiple levels from a library.

I am not entirely happy with the solution here, as there are a bunch of parameters now for different threading modes. @nytamin perhaps you will have some ideas to tidy it up?

@Julusian Julusian requested a review from nytamin March 1, 2020 22:49
Copy link
Owner

@nytamin nytamin left a comment

Choose a reason for hiding this comment

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

I suppose that you're right, this might be a necessary solution in certain cases.

I'm not a fan of requiring the name for all Classes though, perhaps you could add an overloaded function to threadedClass() that, when class-name is omitted, defaults to using myClass.name? That way, it might be backwards compatible as well?

@Julusian
Copy link
Collaborator Author

Julusian commented Mar 3, 2020

I'm not a fan of requiring the name for all Classes though

Maybe so, but this can be hit by a project minifying its code and where threadedclass is used inside a dependency. And the fix will be for that dependency author to supply this parameter. (eg, the reason this was discovered)

I would prefer to instead not pass in the class, and for single threaded mode to load the class in the same way as multithreaded does (to ensure it does behave the same). I did a quick attempt at that but it was really unhappy so I abandoned it for now

@Julusian Julusian force-pushed the feat/exported-name-not-classname branch from 1db0f72 to 82fb4a9 Compare May 12, 2020 15:24
…ded it gets loaded the same way as multithreaded would
@Julusian Julusian force-pushed the feat/exported-name-not-classname branch from 82fb4a9 to f0c0e3f Compare May 12, 2020 15:29
@Julusian
Copy link
Collaborator Author

@nytamin I have reworked this and removed the orgClass parameter instead, as apparently doing that does work without issue.

So the number of parameters is the same, but instead of supplying the class the name of the export is needed.

I think this is much better and safer, as it was possible to get different results in single vs multithreaded due to them getting the class in very different ways

@nytamin nytamin self-requested a review June 8, 2020 09:58
Copy link
Owner

@nytamin nytamin left a comment

Choose a reason for hiding this comment

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

looks good!

@Julusian
Copy link
Collaborator Author

Julusian commented Jun 8, 2020

TODO: Update readme and examples

@Julusian Julusian merged commit 879efa3 into master Jun 8, 2020
@Julusian Julusian deleted the feat/exported-name-not-classname branch June 8, 2020 15:13
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.

None yet

2 participants