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

Bootsie #31

Open
wants to merge 15 commits into
base: master
from

Conversation

@ashryanbeats
Copy link
Member

ashryanbeats commented Nov 5, 2019

Description

New bootstrap option that takes advantage of some of the samples we provide on GitHub.

Usage (any of the following):

$ xdpm bootstrap
$ xdpm bootstrap panel
$ xdpm bootstrap panel my-panel

Plugin type options:

  • headless (default)
  • panel
  • modal
  • react

Motivation and Context

Aiming to make it easier to get started for those who prefer the command line.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@ashryanbeats ashryanbeats requested review from kerrishotts and pklaschka Nov 5, 2019
@pklaschka

This comment has been minimized.

Copy link
Contributor

pklaschka commented Nov 5, 2019

Just an idea (although I realize this might be a slightly bigger undertaking):

If there were some option (may that be via a config file or something else) to add custom repos that should get used, this could be a lot more useful. While I find the samples to be a great reference, they often (and I mean no disrespect by saying this) lack the setup needed for "production-ready" plugins (which is good, as they shouldn't be bloated with config files to be easy to understand, but therefore might not be as useful for real plugins).

I would suspect that users using a CLI to setup a plugin project (including myself) are the type of people who also might use CI services, Linting, Testing etc. For that, plugin boilerplates (as my own) and one's own template can serve as a far more useful starting point when creating a new plugin.

Therefore, while I do very much like the feature and its implementation, I think it would raise this from being "Oh, they've added a feature" to "Ok, that feature sound super-helpful. I'm sure I'm going to use it"...

Also, I have to admit I feel like I'd expect an interactive modal-like CLI experience to pop up when entering the command (such as the one when running npm init), asking me for the manifest.json details.


All of the above is just me adding my two cents 😉. You've asked for feedback, you'll get it...

Apart from that, you've requested my review. As stated before, I'm in no way against the feature or its current implementation, and the above should therefore not get seen as "requests for changes" (which is also why I write this and a comment and will separately approve the changes), but just feedback regarding the feature 🙂 .

package.json Outdated Show resolved Hide resolved
@ashryanbeats

This comment has been minimized.

Copy link
Member Author

ashryanbeats commented Nov 6, 2019

Hi Pablo, this is great feedback!

For both of the following, perhaps we can treat them as feature requests to build on top of the bootstrap command.

1️⃣

I would suspect that users using a CLI to setup a plugin project (including myself) are the type of people who also might use CI services, Linting, Testing etc. For that, plugin boilerplates (as my own) and one's own template can serve as a far more useful starting point when creating a new plugin.

I keep going back-and-forth on this. My original intent here was to provide a CLI feature that makes it super simple for new developers to get started.

But I can see an argument that if someone trying the Plugin APIs already has Node.js/npm installed and wants to bootstrap via the command line, that they may want more than basic boilerplate.

I'm curious to hear if anyone else has thoughts on this.

Either way, I do think we could use what's here as a starting point, and if we decide that bootstrap should provide the bells-and-whistles, we could work on samples specifically intended for the CLI that include linting, testing, etc etc.

2️⃣

I'd expect an interactive modal-like CLI experience to pop up when entering the command (such as the one when running npm init), asking me for the manifest.json details.

We could certainly add this in iteratively (as long as there's a way to bypass it, please).

@yoshikinoko yoshikinoko mentioned this pull request Nov 19, 2019
4 of 10 tasks complete
@yoshikinoko

This comment has been minimized.

Copy link
Contributor

yoshikinoko commented Nov 19, 2019

Hi, sorry for jumping into the thread. This topic is very interesting for me, because of the proposed function is similar to yeoman generator for XD Plugin.

@ashryanbeats

Aiming to make it easier to get started for those who prefer the command line.

I like the idea. Also, I want to remind you that I had a similar motivation to build the yeoman generator.

@pklaschka

I'd expect an interactive modal-like CLI experience to pop up when entering the command (such as the one when running npm init), asking me for the manifest.json details.

I'm interested in the point, which properties do you want to customize while scaffolding?

@pklaschka

This comment has been minimized.

Copy link
Contributor

pklaschka commented Nov 19, 2019

@yoshikinoko

I'm interested in the point, which properties do you want to customize while scaffolding?

I think the main two things would be the plugin id and name. I've stopped counting the times I've overwritten some other plugin project when starting a new project from my boilerplate, when I forget to change the id.

As most other properties have, by now, been moved from the manifest.json to the IO console, things like the description aren't relevant anymore, of course.

My intention with getting interactive there are mainly to get to a "ready for coding" stage as quickly as possible after running the command. The thing is: I have an alias setup in my terminal letting me initialize a plugin based on my boilerplate anywhere (using git clone etc.) with xdinit. Therefore, for this to provide useful functionality for me (or in general, as something added to this should, in my opinion, in the long run be able to do more than a simple command alias in the console 😉 ), it would need to automate common tasks of setting up an XD plugin project as well, including (but not exclusively)

  • Filling the manifest.json fields
  • If there is a package.json file:
    • Update its contents to "reflect" the manifest.json (version, id=>name)
    • run npm install
  • Possibly (re-)initialize a Git repository for the new project

When these tasks are done, I can actually begin programming, meaning if they get automated, the feature benefits my workflow as a developer (requirements here, are, of course, different, meaning an option for configuration would be great for this). Without it, this is a nice little Macro that might be interesting for new users, but won't have any benefits for more experienced users, who probably (like me) already have some sort of efficient workflow to begin new projects...

@ashryanbeats ashryanbeats requested a review from pklaschka Nov 21, 2019
@ashryanbeats

This comment has been minimized.

Copy link
Member Author

ashryanbeats commented Nov 21, 2019

Just pushed a few changes (listed above), if you can have another look @pklaschka @kerrishotts.


@yoshikinoko: point noted about the Yeoman generator. There is going to be room (and appetite) for bootstrappers of various kinds (Pablo has one as well that he linked to above).

Part of the goal here is to come up with a reasonable default within the CLI we already have.

Yeoman is a great option for those who already know the tool or want to learn it. At the same time, there are going to be some developers who may not want to learn/set up xdpm, Yeoman, and the Yeoman plugin. In that case, we'll have a fairly simple default right within xdpm that they can reach for.

commands/bootstrap.js Outdated Show resolved Hide resolved
Copy link
Contributor

pklaschka left a comment

Code-wise, I think everything looks good (apart from the line where I've noted that we'll have to test the behavior on Windows). I do think, however, that the error messages could be a bit more informative... Other than that, everything looks good 🙂

commands/bootstrap.js Outdated Show resolved Hide resolved
commands/bootstrap.js Outdated Show resolved Hide resolved
@ashryanbeats ashryanbeats requested a review from pklaschka Nov 22, 2019
Copy link
Contributor

pklaschka left a comment

I'm currently unable to test this (could do that tomorrow), but since I've tested the version before that and all changes appear to make sense, I'll approve this. The messages are much better now, in my opinion 👍

On the note of testing: Now that GitHub actions are available, I'll probably create a PR sooner or later adding an action to test this for the different operating systems in the future to make this a lot easier 😉

@ashryanbeats

This comment has been minimized.

Copy link
Member Author

ashryanbeats commented Nov 25, 2019

Friendly heads up that I'll likely merge this in later today (EST) or sometime tomorrow.

I'd also like to merge in this minor PR for the v4.0:
https://github.com/AdobeXD/xdpm/pulls

@ashryanbeats ashryanbeats mentioned this pull request Nov 26, 2019
4 of 10 tasks complete
sampleDirs[args[0] || "default"] || sampleDirs.default;
const localDirname = getLocalDirname(args);

if (!shell.which("git")) {

This comment has been minimized.

Copy link
@yoshikinoko

yoshikinoko Nov 26, 2019

Contributor

@ashryanbeats
This is just my humble opinion; I recommend using a node package like https://www.npmjs.com/package/simple-git than executing git installed in the system with shelljs.

External dependency (like the system installed git) would require much complex test-code and need more test cases (e.g., various versions of git). In addition, it is hard for me to read the code of handling error message with shell.echo(> ${stderr}); than console.error()

function checkName(dirname) {
if (!dirname) return;

const unallowedChars = ['"', "'", ";"];

This comment has been minimized.

Copy link
@yoshikinoko

yoshikinoko Nov 26, 2019

Contributor

@ashryanbeats

Windows has more reserved characters
I think you can check the directory name with sanitize-filename

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.