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

[React] Move template related scripts to the base package #1506

Merged
merged 10 commits into from
Jun 6, 2023

Conversation

illiakovalenko
Copy link
Contributor

@illiakovalenko illiakovalenko commented Jun 1, 2023

Description / Motivation

PR includes:

  • Changes from multi-project branch
  • Refactoring, new features for "react" sample:
    • plugins generation
    • component builder, in multi-project branch we used ComponentFactoryCreator, but since we can include something more than just componentFactory (like moduleFactory), we need to use more abstract naming
    • plugins for bootstrap, generate config, component builder
    • changed components tree to represent the same structure like we have in nextjs sample: fields, styleguide, graphql folders. Required for appropriate component scaffold process
  • New utils added to sitecore-jss-dev-tools, including component scaffold, plugins generation, component builder generation, new "react" submodule that can include everything related to react framework
  • Introduced ComponentBuilder as a tool to build components based on the configuration
  • Added more unit tests, documentation, examples for utils

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate)

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)

@illiakovalenko illiakovalenko requested a review from a team June 1, 2023 14:41
/**
* CONFIG GENERATION
*/
require('./generate-config');
Copy link
Contributor

Choose a reason for hiding this comment

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

Would env values for config be read anywhere?
In nextjs we set initial values from env and set the missing values from package.json and scjssconfig.
And would it be better to make bootstrap into plugin too, for parity with nextjs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added env variables for default config
but bootstrap doesn't have plugins (even in nextjs sample app) and I think that we don't really need it since we just need to call a few scripts, maybe let's "pluginize" it if it will be more complex? (like component builder generation)

Copy link
Contributor

@art-alexeyenko art-alexeyenko left a comment

Choose a reason for hiding this comment

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

This is a huge effort, thanks Illia.
I've left some comments for small improvements

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

This looks great!
I'm thinking we should also update the nextjs template to remove duplicated code which is now available in the sitecore-jss-dev-tools package. It appears much of the templating code that was moved is the same in nextjs.

@illiakovalenko
Copy link
Contributor Author

illiakovalenko commented Jun 6, 2023

@ambrauer Agree, "nextjs" also should be updated, but let's update it in a scope of next PBI, since even this part is bigger than was expected initially (we wanted to verify a new approach starting from "react"), and we need to consider doc input + testing. I will create follow-up item

Copy link
Contributor

@art-alexeyenko art-alexeyenko left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

@illiakovalenko Sounds good, as long as the nextjs work item is a fast-follower (we want these to be in the same upcoming release).

@illiakovalenko illiakovalenko merged commit 1ed3dee into dev Jun 6, 2023
1 check passed
@illiakovalenko illiakovalenko deleted the feature/584599-dev branch June 6, 2023 13:22
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.

None yet

3 participants