-
Notifications
You must be signed in to change notification settings - Fork 687
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
Added ability to use custom template for scaffolding. #3025
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ✔️ CodeRevbiew
- ✔️ TestCoverage
- ✔️ Mannual QA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see the default be @magento/venia-concept
if possible. But this is great and allows people to target a specific template version! So simple...why didn't we do this before 🤣
packages/create-pwa/lib/index.js
Outdated
name: 'template', | ||
message: ({ name }) => | ||
`Which template would you like to use to bootstrap ${name}? Defaults to venia-concept.`, | ||
default: 'venia-concept' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a difference between @magento/venia-concept
and venia-concept
? If not maybe we can use the namespaced version since it is similar to how a user would define by version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are maintaining an array which will be matched against first. If that doesn't work, that's when the code looks to pull the package from NPM. The object that we have in that array is looking for venia-concept
. You are right, what you mentioned is what the code does but right now the way the code is laid out works this way:
if template === 'venia-concept`
// use local mapping to find the local venia-concept package
else if template === anything other than `venia-concept`
// check inside mac cache and if it does not find that package, fetch from NPM
// if template is `@magento/venia-concept` use the latest stable version
// if template has a version string attached, use the package with that particular version.
If we change the default to @magento/venia-concept
instead, we won't be able to use local packages while developing. There definitely will be a way to do that, but do we need to spend more time on that? I did think about what you mentioned but opted out personally, but wanted to check with the team. Thanks for bringing this up buddy. Good question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed in dev time and I think we want to be pretty explicit here to avoid the "magic" that helped obscure the release bug.
Basically the target template input text should be the deployed name of the package, as if you were doing a yarn add
ie https://classic.yarnpkg.com/en/docs/cli/add/. So in this case, defaulting to @magento/venia-concept
is fine, but it should default to fetch the latest release version from remote, not local. To use local we would be explicit such as template: file:/path/to/local/folder
.
I think the team had input so if you want to start a thread on slack about it, let's do it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, handled.
await execa.shell(`${buildpackBinLoc} ${argsString}`, { | ||
stdio: 'inherit' | ||
}); | ||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a small change. Moved catch statement to the end of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the call out! You can see this by appending ?w=1
:) https://github.com/magento/pwa-studio/pull/3025/files?w=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preliminary approval so we can get this in QA. The last of my comments have really been minor string changes which I'd like to see updated but no sense in delaying things.
Co-authored-by: Stephen <sirugh@users.noreply.github.com>
@revanth0212 Project built with local (packages/venia-concept) repo has errors after launching Venia and also storybook build fails. |
7529f0e
to
b89cc83
Compare
QA Approved. |
…ento/pwa-studio into revanth/custom_version_label
0269eee
to
ef00b5b
Compare
Description
This PR adds the ability to request the
scaffolding
program to use a custom template/version while creating a new project.Related Issue
Closes PWA-483
Acceptance
Should be able to scaffold a new project using any compatible template on NPM.
Verification Stakeholders
@jimbo @dpatil-magento
Specification
Need to update docs the mention the same.
Verification Steps
Note: This won't work yet with
yarn create @magento/pwa
because we did not publish the new@magento/create-pwa
yet. So let's test this using the new unpublished version locally.node PATH_TO_PWA_STUDIO/packages/create-pwa/bin/create-pwa
Which template would you like to use to bootstrap test? Defaults to venia-concept.
enter any version from NPM. For instance:@magento/venia-concept@9.0.1-alpha.3
. If you don't provide anything, it would check in the file system forvenia-concept
. If it can not find that, then it would search for avenia-concept
in the local machine cache and use that instead.Checklist