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

Update composer.json, move lib files to src folder (renewed) #102

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Update composer.json, move lib files to src folder (renewed) #102

merged 4 commits into from
Sep 22, 2023

Conversation

selfiens
Copy link
Contributor

@selfiens selfiens commented Aug 1, 2023

Related to #67, #101

This PR introduces a Perlite namespace in composer.json and maps it to the newly created /perlite/.src folder.
This enables class autoloading in accordance with PSR-4 standards.

The code changes have been minimized compared to #101.
However, one line in helper.php was modified during local testing
to accommodate OS-independent handling of the temp folder.

I recognize that using .src as a namespace root is unconventional.
While src is more commonly adopted, there's a potential risk of it colliding with a user's vault name (and the vendor folder too).
The prefix . in src is intended as a temporary measure until the project undergoes a folder structure reorganization.
I welcome any suggestions or feedback.

@secure-77
Copy link
Owner

secure-77 commented Aug 1, 2023

Thank you again for this PR,

On more thing comes to my mind: is it possible to specify the path for the autoloader on the build? In my current .gitignore I have an entry for the vendor folder to avoid pushing lots of unnecessary files to this repo, but I want that a clone of this repo will also work without building it via composer, also I need to take care to copy only necessary files to the docker image on the build process.

So my question is, if we introduce a namespace and a new sub folder (vendor) for the autoloader.php can we change the structure that we only need one new sub folder for autoloader.php, PerliteParsedown.php, Parsedown.php etc.

So I would recommend to either move also the PerliteParsedown.php to the vendor folder and I change the .gitingore to exclude everything non needed from the vendor folder or we go the other way to have a new (src) folder and move the autoloader.php PerliteParsedown and Parsedown.php to this folder.

You know what I mean?

@selfiens
Copy link
Contributor Author

selfiens commented Aug 2, 2023

Thank you for taking the time to review my PR :)

Your concerns are very thoughtful. I completely support your ideas.
End users should be able to git clone without any extra steps.

Below are brief pros and cons of including/excluding the vendor folder.

Criteria Including vendor Excluding vendor
Newcomer Experience Easier setup for beginners. Requiring Composer can be a big huddle.
Dev Experience Acceptable, following the nature of the project. Common practice. But some devs are not familiar with Composer.
Consistency Everyone uses the exact same set of libraries. Possible variation in library versions if not managed properly.
Offline Development Possible. Requires internet connection initially to fetch dependencies.
Repository Size Increased size due to all dependency files. Only your project's direct code.
Security Risk if malicious code gets into vendor and goes unnoticed. vendor is managed by Composer, still need to trust 3rd party.

Based on my understanding, your question is about having one folder to store both 1st and 3rd party class files.

  1. Hosting 1st + 3rd party class files in a single folder.
    • The composer vendor folder is designated for 3rd party files, and is considered volatile.
      • Anyone can delete the folder entirely and rebuild it from scratch.
      • Therefore, storing project's class files directly inside the vendor folder would be problematic.
    • You might be suggesting the following folder structure, which seems like a feasible option:
      • /perlite/.lib (for example)
        • /src - project's unique classes
        • /vendor - composer's vendor
  2. Changing the name and/or location of the vendor folder (e.g. .lib or .vendor):
    • The composer.json file offers an option for this.
    • Experienced PHP devs might find this confusing.
    • An alternative option is to separate the web root folder.

Here are my opinions:

  • Including the vendor folder in the git for easier installation is a good idea.
    • after removing JS dependencies, the size gets considerably smaller.
  • I don't recommend mixing the project's unique files with 3rd party files.
  • The name vendor is changeable, but we should proceed with caution.

My view on a more common folder structure is:

/perlite
    /public - web root
        /favicon.ico
        /.js
        /.styles
        /index.php
        /(user vaults)
    /src - project's unique classes
    /vendor
    /composer.json

Developers familiar with Composer or those coming from popular frameworks will feel comfortable with this structure.

The integration of Composer entails significant decisions.
However, I believe in the value of integrating Composer for the future of this project.
I'll answer your questions to the best of my knowledge to help you make the right decision.
Any additional questions, new ideas, or entirely different approaches are welcome :)

@secure-77
Copy link
Owner

Thank you for the extensive information! I think I will follow your suggestions. Unfortunately, this means I need to exclude the JS dependencies again from the composer and either update them manually, use CDNs or use a second composer file and a bash script. However, if there is a best practice in development, I will try to follow it :) I will try to include this in the next release.

@secure-77 secure-77 merged commit fc39baa into secure-77:dev Sep 22, 2023
1 check passed
@secure-77
Copy link
Owner

I decided to use a separate php composer file for the js assets. This way I can include the vendor folder and use the php autoload script. Release is now online in 1.5.7.

Thanks again for the contribution!

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.

2 participants