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

[Extensibility 🤌🏻] Make create-app work with "extended templates" #1205

Merged
merged 327 commits into from
May 26, 2023

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented May 12, 2023

Description

Whats included in this PR?

  • Removal of the my-extended-retail-app template project (I'll talk about why below 👇🏻)
  • Addition of "bootstrap" generator asset/template living in the pwa-kit-create-app project
  • Changes to the generator itself to support handlebars templates.
  • Changes to the generator to enable downloading of templates from NPM and their extraction to the destination project folder.
  • Changes to the generator to ask whether or not you'd like to use template extensibility.
  • Updated the dependencies in the template-retail-react-app from dev to regular dependencies so that they would get installed when generating the project.
  • Slimmed down the amount of customization in the extended application.

3PP

Added handlebars the version in use already had an existing 3pp approval here.

Why did I remove the my-extended-retail-app.

Here are the reasons why having another projects template that is use essentially as a wrapper for the template-retail-react-app isn't idea.

  1. The expectation that developers have when using template extensibility is to have a single dependency, the template that they are extending (maybe some peer deps but I haven't thought deeply on that). Right now if you were to generate the my-extended-retail-app you'd have a very large package.json file.
  2. There was a lot of duplicate code. As mentioned the package.json in its entirety was duplicated. Meaning we would have to now maintain 2 places when making changes to the template-retail-react-app.
  3. Having a think template project in the monorepo as a solution wouldn't really scale well. For example if we have N more templates that support extensibility in the future that would mean we would also have to have N more thin templates used for generation, and N more file duplication. Future multi-extensibility (e.g. extending from multiple projects) would further exacerbate the issue.
  4. We can't solve the duplicate package.json file problem without making the my-extended-retail-app un-runnable in the monorepo. What this means, is that if we change the dev dependancies in the template-retail-react-app as we should, because lerna uses symbolic links for dependencies that are within the monorepo, the package resolution doesn't work correctly.
  5. Nice to have: Fix the router override so we can have a simple import, modify, export pattern and not import, modify, configure, export that we have to do now.
  6. Nice to have: The demo project will have a sample new extended page that talked about extensibility on it, it should open on start of the dev server once, then not after.

How to Test-Drive this PR 🚘

It's very important that we test all the presets that the generator allows you to use. Below are the command you can run to locally create projects. For each command below we want to make sure that where are no errors generating, that the project generated is the one expected and the files expected, with the configuration expected. You should also run npm start for each to give. it a light smoke test.

Before you run any of the below commands to generate projects, ensure you delete your npx cache:

> rm -Rf ~/.npm/_npx  

Additional Notes:

When choosing to generate the retail react app using the "demo" instance, extensibility will be used by default. The reason for doing this is 2 fold. First the demo preset isn't intended to be used as the project starting point for a partners project, it's intended to boot up the retail react app quickly so you can demo the site to someone, and not iterate on the code. Secondly, CI currently uses this preset to test that project generation works, we have to make sure that we don't block this preset by waiting for questions to be answered, this would break CI.

# No preset, select the option to use you own instance. Try both Yes and No to extensibility.
> node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir generated-project

# Generates a `retail-react-app` demo project, no need to answer questions, this will be created using extensibility
> node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir generated-project --preset=retail-react-app-demo

# Generates a `retail-react-app` test project, no need to answer questions, this will be created using extensibility
> node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir generated-project --preset=retail-react-app-test-project

# Generates a `typescript-minimal` test project, no need to answer questions, this will be create via the bundle asset
> node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir generated-project --preset=typescript-minimal-test-project

# Generates a `typescript-minimal` project, answer questions, this will be create via the bundle asset
> node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir generated-project --preset=typescript-minimal

# Generates a `express-minimal` test project, no need to answer questions, this will be create via the bundle asset
> node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir generated-project --preset=express-minimal-test-project

# Generates a `express-minimal` project, answer questions, this will be create via the bundle asset
> node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir generated-project --preset=express-minimal

TODO's

  • Template the "scripts" portion of the package.json file to remove that duplication.
  • Refactor the generator to make it easier to understand and change in the future.
  • Correctly version the extended template version in the bootstrap assets
  • Clean up naming/folder structure
  • Fix generated project package name being empty
  • Set package.json version number for all generated projects.
  • Add preset specific assets and process them if folder is found.
  • Think about the versioning of the extended template, might not have to do anything here.'
  • Re-add the manifest stuff.
  • Update code with base branch.
  • Test bundle-size. // NOTE: Still over limit of 275kb but only by 25k (probably from the commerse-sdk-reack)
  • Update PR description.
  • Do a sweep for any features that were removed that I need to re-add.
  • 11. Consume translation work.
  • 12. Scan retail-react-app dependencies and sort if needed.
  • 13. Do todos and remove comments.
  • 14. Fill empty JSDoc method descriptions.
  • 15. Make sure that CI is passing.

bfeister and others added 30 commits April 6, 2023 10:07
* v3:
  Drop npm 6 and node 14 from V3 (#1112)
  Upgrade lerna (#1111)
  add locale and currency to apiClients (#1107)
  Update test.yml (#1109)
  [V3] Invoke push action for v3 to `staging-v3` target (#1108)
  remove commerce api folder (#1105)

# Conflicts:
#	package-lock.json
#	package.json
#	packages/commerce-sdk-react/package-lock.json
#	packages/commerce-sdk-react/package.json
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/pwa-kit-create-app/package.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-dev/package.json
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/template-retail-react-app/app/mocks/mock-data.js
#	packages/template-retail-react-app/package-lock.json
#	packages/template-typescript-minimal/package-lock.json
#	packages/test-commerce-sdk-react/package-lock.json
* v3:
  Sort contents of package.json files. (#1115)
  Fail fast? False! (#1116)

# Conflicts:
#	packages/pwa-kit-create-app/package.json
#	packages/pwa-kit-dev/package.json
#	packages/pwa-kit-runtime/package.json
#	packages/template-typescript-minimal/package.json
…-rehaul

* feature/template-extensibility:
  drop unnecessary pages/product-detail override
  updates from underlying template-retail-react-app
  add missing useToast (bad merge)
  add missing node engines
  update spike project's engines / regen v3 lockfiles
  resolve merge conflict
  merge conflict

# Conflicts:
#	packages/spike-extendend-retail-app/pwa-kit/overrides/app/components/header/index.jsx
#	packages/spike-extendend-retail-app/pwa-kit/overrides/app/pages/product-detail/index.jsx
…ckly toggling some base constant values (e.g. categories shown on home page)
@bendvc bendvc changed the base branch from feature/template-extensibility-algo-refactor to develop May 25, 2023 16:30
@vcua-mobify
Copy link
Collaborator

Small nit that we don't need to act on now: Can we make the prompt clearer on character limits?

ie. test-template-extensibiltly is too long for the project name
Screenshot 2023-05-25 at 3 44 35 PM

ie. test-template-extend is allowed
Screenshot 2023-05-25 at 3 45 44 PM

Copy link
Collaborator

@bfeister bfeister left a comment

Choose a reason for hiding this comment

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

Right now, the my-extended-retail-app directory is still marked for deletion. I'm not sure the last time you merged back from develop, but if needed we want to revert your deletion. We'll handle that deletion in a follow-up PR when we've done more testing and done more side-by-side comparison of generated (outside monorepo) projects to the my-extended-retail-app.

@bfeister bfeister requested review from adamraya and bfeister May 26, 2023 20:40
@bfeister
Copy link
Collaborator

bfeister commented May 26, 2023

We are merging this without CI Windows passing. This is a known issue, but must be resolved for v3 to launch. A new GUS ticket is open here to track fixing this prior to an official / feature-complete v3 release:

W-13493426

@bendvc bendvc merged commit 0a22c15 into develop May 26, 2023
bfeister added a commit that referenced this pull request May 30, 2023
* develop:
  Versioning the retail react app (#1209)
  [V3] Add `templateVersion` arg to `create-react-app` (#1229)
  [Extensibility 🤌🏻] Make `create-app` work with "extended templates" (#1205)
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.

7 participants