Skip to content

Conversation

reemardelarosa
Copy link
Contributor

@reemardelarosa reemardelarosa commented Dec 6, 2022

#7

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

Add preset generator to plugin qwik-nx.

Use cases and why

npx create-nx-workspace org-workspace --preset=qwik-nx

Screenshots/Demo

https://github.com/qwikverse/qwiknx-workspace

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@shairez
Copy link
Contributor

shairez commented Dec 8, 2022

Hey @reemardelarosa !
Thanks for this PR

2 questions:

  1. when do you think it'll be ready to merge?
  2. can you please update the README with an example of how to use the present?

Thanks 🙏

create nx with qwik-nx preset
@reemardelarosa
Copy link
Contributor Author

@shairez

  1. Tomorrow/Sunday it will be ready for merge.
  2. Added documentation in readme

@shairez
Copy link
Contributor

shairez commented Dec 9, 2022

thanks brother! 🙏

reuse application generator codes and interfaces
@reemardelarosa reemardelarosa changed the title [WIP] feat: add "preset" generator feat: add "preset" generator Dec 10, 2022
reuse addFiles from application generator
Copy link
Contributor

@dmitry-stepanenko dmitry-stepanenko left a comment

Choose a reason for hiding this comment

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

@reemardelarosa sorry, maybe I didn't explain it correctly: you don't need any logic at all in this generator

Here's how this file can look like while preserving all the functionality

import { Tree } from '@nrwl/devkit';
import { QwikWorkspacePresetGeneratorSchema } from './schema';
import applicationGenerator from '../application/generator';

export default async function (
  tree: Tree,
  options: QwikWorkspacePresetGeneratorSchema
) {
  return applicationGenerator(tree, options);
}

Please also remove extra exports in application generator

We may add something else to the preset generator later if it's needed, but at this point we can only create an app 🤷‍♂️

reuse existing generator and remove unnecessary codes
@reemardelarosa
Copy link
Contributor Author

@dmitry-stepanenko Updated

tree: Tree,
options: QwikWorkspacePresetGeneratorSchema
) {
options.directory = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

this makes directory property basically useless. Is it intentional?

Copy link
Contributor Author

@reemardelarosa reemardelarosa Dec 11, 2022

Choose a reason for hiding this comment

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

@dmitry-stepanenko

function normalizeDirectory(options: QwikAppGeneratorSchema) {
return options.directory
? `${names(options.directory).fileName}/${names(options.name).fileName}`
: names(options.name).fileName;
}

options.directory = '' will use the names(options.name).fileName;

There is a bug if I will just reuse the options.directory, it will create org/apps/qwik-app/qwik-app.

Do you have any suggestions to fix it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot reproduce this, can you please describe how exactly this comes out?

Copy link
Contributor Author

@reemardelarosa reemardelarosa Dec 11, 2022

Choose a reason for hiding this comment

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

have you tried to run npx create-nx-workspace org-workspace --preset=qwik-nx ?
This is the result of

export default async function (
  tree: Tree,
  options: QwikWorkspacePresetGeneratorSchema
) {
  return applicationGenerator(tree, options);
}

Screenshot 2022-12-11 at 8 30 50 PM

Copy link
Contributor

@dmitry-stepanenko dmitry-stepanenko Dec 11, 2022

Choose a reason for hiding this comment

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

Hm, it's not reproducible for me if I run npx create-nx-workspace org-workspace --preset=qwik-nx
image

But I can actually reproduce by running npx nx generate qwik-nx:preset abc --no-interactive --dry-run in an existing repo by replacing actual options with the list of options that is being provided to the preset generator by create-nx-workspace:

{ "_": ["neatspace", "Object]"], "preset": "qwik-nx", "allPrompts": false, "a": false, "skipGit": false, "g": false, "commit": "[object", "/bin/sh": "/Users/dmitriy/.npm/_npx/d2207cf76adb22dc/node_modules/.bin/create-nx-workspace", "name": "neatspace", "appName": "", "nxCloud": false, "packageManager": "npm", "defaultBase": "main", "ci": "", "cli": "nx", "style": "css", "skipInstall": false, "linter": "eslint", "directory": "neatspace", "presetVersion": "undefined", "skipFormat": false, "unitTestRunner": "vitest", "strict": true }

Since nx may use/override flags that we're using, it feels like the best what we can do is to modify the schema to have other names there. For example, it can properly generate the app name using this command npx create-nx-workspace --preset=qwik-nx --qwikAppName=neat-qwik-app.

Here's the list of inputs that will be overridden:

      name,
      preset,
      appName,
      style,
      cli,
      nxCloud,
      packageManager,
      defaultBase,
      ci,

So I propose to rename the following inputs:

  • name => qwikAppName ?? options.name
  • style => qwikAppStyle

and remove the following inputs as they won't be actually used:

  • directory
  • skipFormat

Then we can map property names to what applicationGenerator needs.

@reemardelarosa @shairez maybe you have any other thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@shairez @reemardelarosa I think it's better to ship whatever we can at this point and enhance it later down the road once we're able to

Copy link
Contributor

Choose a reason for hiding this comment

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

@reemardelarosa did you have a chance to implement this yet?

#10 (comment)

if not please let us know and we'll do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with @dmitry-stepanenko, to create separate issue/pr for standalone.
WDYT @shairez ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@reemardelarosa yes, standalone could be a separate PR
I was talking about the link in my last comment, about implementing the changes you suggested in your last comment

wanted just to ask if you did that work already and didn't push, because I didn't see it in the commits yet and didn't want us to do duplicated work

Thanks!

Copy link
Contributor Author

@reemardelarosa reemardelarosa Dec 14, 2022

Choose a reason for hiding this comment

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

@shairez I did not implemented this yet

name => qwikAppName ?? options.name
style => qwikAppStyle

@shairez
Copy link
Contributor

shairez commented Dec 12, 2022

@dmitry-stepanenko @reemardelarosa

WDYT about adding a prompt asking if they want the new "standalone" project vs the traditional monorepo project?

@dmitry-stepanenko
Copy link
Contributor

Thanks @reemardelarosa!

@dmitry-stepanenko dmitry-stepanenko merged commit 9947c6b into qwikifiers:main Dec 15, 2022
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