adrien2p / medusa-extender

:syringe: Medusa on steroid, take your medusa project to the next level with some badass features :rocket:
https://adrien2p.github.io/medusa-extender/
MIT License
320 stars 38 forks source link

feat: added async loading support to medusa #139

Closed SGFGOV closed 1 year ago

SGFGOV commented 1 year ago

added support to async load modules fixes issue #140

SGFGOV commented 1 year ago

@adrien2p can you please review this

adrien2p commented 1 year ago

Thank you for the contribution. I ll have a look as soon as I can 🚀

adrien2p commented 1 year ago

have you tested it? cause medusa will still strip the properties from the config no?

SGFGOV commented 1 year ago

yes.. the properties you need have to be inside the project config.

Best Regards Govind

On Thu, 20 Oct 2022, 18:33 Adrien de Peretti, @.***> wrote:

have you tested it? cause medusa will still strip the properties from the config no?

— Reply to this email directly, view it on GitHub https://github.com/adrien2p/medusa-extender/pull/139#issuecomment-1285505024, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXEQJHHNCHYMCYEKZUYLSFDWEE7KVANCNFSM6AAAAAARIAIYCM . You are receiving this because you authored the thread.Message ID: @.***>

--

This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.   

adrien2p commented 1 year ago

Will you be able to tackle the comments?

SGFGOV commented 1 year ago

have you tested it? cause medusa will still strip the properties from the config no?

yup, you need to encapsulate settings inside project config

adrien2p commented 1 year ago

Could you add everything related to vscode in the gitignore as well as add unit tests for the async load with config that also includes sync/async function to resolve?

SGFGOV commented 1 year ago

Could you add everything related to vscode in the gitignore as well as add unit tests for the async load with config that also includes sync/async function to resolve?

done

adrien2p commented 1 year ago

Could you add everything related to vscode in the gitignore as well as add unit tests for the async load with config that also includes sync/async function to resolve?

done

Ahah thanks, but it is still not exactly how it should be done. The tests for the utility should be in a separate file as it concerns the util and not the cli command it self.

A bit more structure in the tests would be nice.

The fake config data can be moved to a fixtures directory in the tests dir like it is done in other places. You can then create different config files for the different use case and just load them without having to writeRile each time and without having hard readable dummy config 🙏

Can you clean up please?

After that we should be good or I will directly write the proposed changes so that you can either do it or accept it depending on the type of changes.

Thanks a lot again 👌

SGFGOV commented 1 year ago

Owner

Thanks a lot Adrien :), I'll be obliged if you can help with the clean up.

SGFGOV commented 1 year ago

@adrien2p i've refactored as requested

adrien2p commented 1 year ago

@adrien2p i've refactored as requested

Ahah this is not that yet 😂 sorry. I ll write you the changes asap

adrien2p commented 1 year ago

I think that after resolving the last few comments we should be good to merge

adrien2p commented 1 year ago

@SGFGOV let me know when you have time to finish it 😀

adrien2p commented 1 year ago

@SGFGOV should I consider that pr as dead?

SGFGOV commented 1 year ago

@SGFGOV should I consider that pr as dead?

Hi Sorry, I've just been held up the last two weeks. No its not dead, I'll finish it in a day or two. Sorry again

adrien2p commented 1 year ago

Thanks for your work 🚀 could I get you to run the linter please. Also, the tests need to be fixed

SGFGOV commented 1 year ago

I think the github action permissions needed to be changed as we were writing a file in the earlier implementation, I've modified it so that optionally you can pass a file to the loader (as you had suggested right in the beginning :-) . I didn't understand why you said that then. Now I do 👍 ) and also modified the tests. Now the tests are also in line with the expected behaviour.

adrien2p commented 1 year ago

Looks much better 🥰 happy that you finally see what i was aiming for 🚀 nice work man have you been able to lint as well?

adrien2p commented 1 year ago

@SGFGOV ? I d like to merge it this week and release a new version

SGFGOV commented 1 year ago

sure

SGFGOV commented 1 year ago

yes I did lint, Thanks for your infinite patience :)

adrien2p commented 1 year ago

@SGFGOV Let me know when you have the time for the lasts clean up stuff so that I merge it :) and the tests need to be fixed

adrien2p commented 1 year ago

@SGFGOV any news?

SGFGOV commented 1 year ago

my apologies..have been under the weather the past couple of days

adrien2p commented 1 year ago

Looks like something went wrong with your merge

SGFGOV commented 1 year ago

yeah .. looks like something went grossly wrong, let me close and reopen the pr on a fresh branch