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.

Sign up for GitHub

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

Jump to bottom

feat: add module loader #8

Merged
merged 9 commits into from
Mar 7, 2025
Merged

feat: add module loader #8

merged 9 commits into from
Mar 7, 2025

Conversation

leogermani
Copy link
Contributor

No description provided.

ryanwelcher
ryanwelcher requested changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@ryanwelcher ryanwelcher left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just had a couple of questions and a suggestion.

@leogermani leogermani marked this pull request as ready for review March 5, 2025 21:31
@leogermani leogermani requested a review from ryanwelcher March 5, 2025 21:31
@leogermani leogermani self-assigned this Mar 6, 2025
@ryanwelcher
Set for ps-4 autoloading. Fix a small bug where the options array is …
8be1451 
…not an array after clearing the enabled ones.
@ryanwelcher
Copy link
Contributor

@leogermani I got PSR-4 loading working and added a small fix. It required that we do some renaming. Mind having a look?

@leogermani
Copy link
Contributor Author

@leogermani I got PSR-4 loading working and added a small fix. It required that we do some renaming. Mind having a look?

Thanks @ryanwelcher , I thought that is exactly what I did 🤷

But to get it fully working I had to do a bit more of renaming -> 897c33e

Looks good to me. The only thing that makes me have second thoughts is that this is not what we use in all other Newspack plugins... but not a big deal tbh

@ryanwelcher
Copy link
Contributor

Looks good to me. The only thing that makes me have second thoughts is that this is not what we use in all other Newspack plugins... but not a big deal tbh

If you think this is an issue, maybe we need to discuss it further? I don't want to delay this further than needed but also want to set the publishers up for success. cc @claudiulodro

@ryanwelcher
Copy link
Contributor

Closes #10

@leogermani leogermani merged commit da1199f into trunk Mar 7, 2025
3 checks passed
@ryanwelcher ryanwelcher mentioned this pull request Mar 10, 2025
Closed
@ryanwelcher ryanwelcher deleted the add-module-loader branch March 10, 2025 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanwelcher ryanwelcher ryanwelcher approved these changes

Assignees

@leogermani leogermani

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@leogermani @ryanwelcher