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

Add new docs site #37

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add new docs site #37

wants to merge 2 commits into from

Conversation

tobyzerner
Copy link
Contributor

@tobyzerner tobyzerner commented Dec 26, 2023

This PR adds a new docs site powered by VitePress. The docs become part of the source repo, rather than in a separate repo, which is easier for users to find and keeps them versioned together with the library.

I've mostly copy/pasted from the existing site, with the most substantial change to the User Guide which is now titled Quick Start. The instructions on this page are currently hypothetical - some ideas for what the UX of Porter could look like - hence this PR is a draft and should not yet be merged. Specifically:

  • Now that Porter is exclusively a CLI tool, composer global require is the recommended installation method
  • Add an porter init command to generate a new config file
  • Changes to the config file - make connections a keyed array and use database config keys directly from Illuminate. (Not sure if the other config options are still needed?)
  • I envision the run command will allow you to select platforms from a list (rather than having to know the name to type)
  • I envision the run command will automatically use database connections named source and target if they are present

Let me know what you think about this, I'm keen to contribute these changes if they sound okay.

A GitHub action is included which will deploy the docs to GitHub Pages for the repo. See here for how to configure this to work.

Preview the docs locally by running:

npm i
npm run docs:dev

@tobyzerner tobyzerner marked this pull request as draft December 26, 2023 05:01
@tobyzerner tobyzerner changed the title Draft: Add new docs site Add new docs site Dec 26, 2023
@linc
Copy link
Owner

linc commented Dec 26, 2023

This mostly sounds good but I have a couple notes.

I envision the run command will allow you to select platforms from a list (rather than having to know the name to type)

I currently have porter list sources as the way to get the input for the run command. The list is currently so long it's going to run the prompt off the screen if you do it interactively. I'm not necessarily opposed, I'm just unclear it's a UX win with a list that long. Today at least it's a much clearer win for targets with only 3 on the list, but then you have non-parallel behavior if one's a text input and the other's multiple choice. Any ideas?

Side point: There's a difference between each platform's "full name" and its class name. We'd want the full name in a list like that for clarity, but then we'd need to map it in code somewhere. The current list option shows the slug in bold so it's clear you're supposed to type the simplified version that maps for us.

Changes to the config file - make connections a keyed array and use database config keys directly from Illuminate. (Not sure if the other config options are still needed?)

The only reason it isn't keyed today is I believe it adds complexity for manually editing, so that part's fine if it's being generated. However, I'm not keying directly to Illuminate currently because I want to be able to use the same config to use an API connection as the source or target in the future. Today it's all database-driven but I didn't want to paint myself into a corner with the naming. That's why they are "connections" and not "databases". I suspected that creating a separate way to store API connections adds more complexity than a slug translation step for Illuminate.

@tobyzerner
Copy link
Contributor Author

Good point! My suggestion would be to make use of Laravel Prompts, which allows you to select from a scrollable list. The options array's values are displayed and the key for the selected option is returned.

$source = select(
    label: 'Source package',
    options: array_map(fn($package) => $package['name'], \Porter\Support::getInstance()->getSources()),
);

I would also generally support using Laravel Prompts for other interactivity within the commands, it's really nice UX. Also happy to contribute this!


With the config - future-proofing is a valid concern. But I think that for different types of connections (database, API, etc) you're going to end up with vastly different config options anyway, and any overlap is kind of coincidental so I don't think there's any need to aim for consistency, apart from where it makes sense. Think of it like this:

'foo' => [
    'type' => 'database',
    // database config options
],

'bar' => [
    'type' => 'api',
    // api config options
]

and then:

$type = $config['type'];
unset($config['type']);

$connection = match ($type) {
    'database' => new DatabaseConnection($config),
    'api' => new ApiConnection($config),
    default => throw new Exception('unsupported connection type'),
};

Make it each connection type implementation's responsibility to handle whatever config it's expecting.

In reality, the Illuminate database config actually supports a lot of other options that may be useful too, so now we can pass them directly in without needing to map to any arbitrary format:

'source' => [
    'type' => 'database',
    'driver' => 'mysql',
    'url' => '',
    'host' => '127.0.0.1',
    'port' => '3306',
    'database' => 'database',
    'username' => 'root',
    'password' => 'password',
    'unix_socket' => '',
    'charset' => 'utf8mb4',
    'collation' => 'utf8mb4_0900_ai_ci',
    'prefix' => '',
    'prefix_indexes' => true,
    'strict' => true,
    'engine' => null,
    'options' => [
        PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => false,
    ],
],

And your API connection can take whatever shape makes sense, maybe something like:

'target' => [
    'type' => 'api',
    'host' => 'api.example.com',
    'headers' => [
        'Authorization' => 'Bearer a1b2c3',
    ],
    // whatever else
]

@linc
Copy link
Owner

linc commented Dec 31, 2023

That all sounds good to me. I see laravel/promtps uses symfony/console as a dependency, so might as well convert to use that underlying package as well, assuming it's comparable to the one I grabbed.

@vesper8
Copy link

vesper8 commented Feb 24, 2024

import './custom.css'; is causing an error because that file does not exist

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.

3 participants