Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

Resolution for @types/react for checkout ui ext #315

Closed
wants to merge 3 commits into from

Conversation

jamesvidler
Copy link
Contributor

Fixes #290.

Problem
When creating a new typescript react checkout extension, all react components show typescript errors.

Solution
Added a specific resolution to the package.json template for "@types/react": "17.0.2". This will only be added if the extension being created is Typescript + React.

Bonus: added a test for this as well.

馃帺Tophatting

  1. Clone both this repo and shopify-cli.
  2. In shopify-cli-extensions checkout this patch.
  3. In shopify-cli-extensions run make boostrap.
  4. In shopify-cli run bundle install followed by rake extensions:symlink (this will link your local shopify-cli-extensions build.
  5. Run alias shopifyls="SHOPIFY_CLI_DEVELOPMENT=1 SSL_VERIFY_NONE=1 $HOME/src/github.com/Shopify/shopify-cli/bin/shopify"
  6. Run shopifyls extension create and select typescript-react as the option for a checkout extension.
  7. Open the code and verify you have no typescript errors.

Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

I think the eventual solution to this will just be to upgrade @remote-ui/react to depend on react^18.0.0, but I am not sure when we will be able to get around to that, so I like this fix in the meantime. Left a couple of notes, though!

create/templates/shared/package.json Outdated Show resolved Hide resolved
@@ -14,5 +14,8 @@
"scripts": {
"build": "shopify-cli-extensions build",
"develop": "shopify-cli-extensions develop"
}
}{{ if and .Development.UsesTypeScript .Development.UsesReact }},
"resolutions": {
Copy link
Member

Choose a reason for hiding this comment

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

resolutions is just for Yarn. I don't know what the current CLI's policy is on supporting other package managers, but I know for CLI 3.0 they intend to support the "big three" (npm, pnpm, yarn). npm and pnpm both use overrides instead. Is it possible for us to detect and provide only the correct field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this is possible:

func (path InstallDependencies) Run() error {
var package_manager string
if yarn, err := LookPath("yarn"); err == nil {
package_manager = yarn
} else if npm, err := LookPath("npm"); err == nil {
package_manager = npm
} else {
return errors.New("package manager not found")
}
cmd := Command(string(path), package_manager)
output, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("failed to install dependencies: %s", output)
}
return nil
}

We can probably refactor the package_manager resolver and use the same logic when building the package.json.tpl (with the package_manager passed as an argument to the template).

@jamesvidler jamesvidler self-assigned this May 19, 2022
@jamesvidler
Copy link
Contributor Author

Worth noting this may not be a critical priority for editions if we do not ship with Typescript template support.

@jamesvidler
Copy link
Contributor Author

jamesvidler commented Jun 24, 2022

@lemonmade I think we can close this PR now since its no longer relevant to CLI 3.0? Do we have this captured elsewhere for CLI 3.0?

@lemonmade
Copy link
Member

Yeah we can close this now (馃槩)

@jamesvidler
Copy link
Contributor Author

Closing since this is no longer relevant.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix React compilation errors in checkout extension template
3 participants