-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Support for scoped templates #21615
Support for scoped templates #21615
Conversation
Add support for templates published as scoped packages. Partially fixes #18973.
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
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.
Code analysis results:
eslint
found some issues.
local-cli/generator/templates.js
Outdated
@@ -75,7 +75,13 @@ function createFromRemoteTemplate( | |||
templateName = template.substr(template.lastIndexOf('/') + 1); | |||
} else { | |||
// e.g 'demo' | |||
installPackage = 'react-native-template-' + template; | |||
let scope = '' |
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.
prettier/prettier: Insert ;
local-cli/generator/templates.js
Outdated
@@ -75,7 +75,13 @@ function createFromRemoteTemplate( | |||
templateName = template.substr(template.lastIndexOf('/') + 1); | |||
} else { | |||
// e.g 'demo' | |||
installPackage = 'react-native-template-' + template; | |||
let scope = '' |
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.
semi: Missing semicolon.
local-cli/generator/templates.js
Outdated
@@ -75,7 +75,13 @@ function createFromRemoteTemplate( | |||
templateName = template.substr(template.lastIndexOf('/') + 1); | |||
} else { | |||
// e.g 'demo' | |||
installPackage = 'react-native-template-' + template; | |||
let scope = '' | |||
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { |
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.
keyword-spacing: Expected space(s) after "if".
local-cli/generator/templates.js
Outdated
@@ -75,7 +75,13 @@ function createFromRemoteTemplate( | |||
templateName = template.substr(template.lastIndexOf('/') + 1); | |||
} else { | |||
// e.g 'demo' | |||
installPackage = 'react-native-template-' + template; | |||
let scope = '' | |||
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { |
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.
prettier/prettier: Replace (template[0]·===·'@'·&&·template.length·>·3·&&·template.includes('/',·2)
with ·(⏎······template[0]·===·'@'·&&⏎······template.length·>·3·&&⏎······template.includes('/',·2)⏎····
local-cli/generator/templates.js
Outdated
installPackage = 'react-native-template-' + template; | ||
let scope = '' | ||
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { | ||
[scope, ...template] = template.split('/') |
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.
prettier/prettier: Insert ;
local-cli/generator/templates.js
Outdated
installPackage = 'react-native-template-' + template; | ||
let scope = '' | ||
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { | ||
[scope, ...template] = template.split('/') |
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.
semi: Missing semicolon.
local-cli/generator/templates.js
Outdated
let scope = '' | ||
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { | ||
[scope, ...template] = template.split('/') | ||
scope += '/' |
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.
prettier/prettier: Insert ;
local-cli/generator/templates.js
Outdated
let scope = '' | ||
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { | ||
[scope, ...template] = template.split('/') | ||
scope += '/' |
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.
semi: Missing semicolon.
local-cli/generator/templates.js
Outdated
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { | ||
[scope, ...template] = template.split('/') | ||
scope += '/' | ||
template = template.join('/') |
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.
prettier/prettier: Insert ;
local-cli/generator/templates.js
Outdated
if(template[0] === '@' && template.length > 3 && template.includes('/', 2)) { | ||
[scope, ...template] = template.split('/') | ||
scope += '/' | ||
template = template.join('/') |
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.
semi: Missing semicolon.
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@scally, thanks for this PR! Do you have a link to the scoped template you published? I'd love to test and land this, as soon as possible. 😁 |
@RSNara I don't have a link at hand, as I tested with a private repo. You could publish the existing RN default template as a scoped package, or I could try to do the same soon. |
@RSNara I uploaded a public sample scoped template you can use to test with.
|
@RSNara Could you please take a look at this & the provided example when you have a second? I don't want to see this PR get closed like the last one fixing this issue. :) |
@RSNara bump 😸 |
@lucianomlima @TheSavior @hramos Mentioning some other folks that worked on this file in the hopes it can see some traction. @RSNara said above he wanted to land this ASAP, but I haven't heard from him in a month. Is there anyone else that could verify or sign off on this fix? I have provided a sample publicly scoped template to test with above:
|
cc @grabbou since this is CLI stuff |
Thank you for your PR. We just moved this code to https://github.com/react-native-community/react-native-cli so please submit your PR there. |
Can someone tell me if there's some pull-request for this feature at |
We haven't merged it and I feel really bad that we have waited with it for so long. Let's actually make sure this is working. CC: @Esemesek since you are working on |
Summary: - **[0eea57724](facebook/react@0eea57724 )**: Fix typo in comment (accumlated → accumulated) ([#21637](facebook/react#21637)) //<ithinker5>// - **[0706162ba](facebook/react@0706162ba )**: Fix typo in comment (environement → environment) ([#21635](facebook/react#21635)) //<niexq>// - **[9d17b562b](facebook/react@9d17b562b )**: Fix typo in comment (satsify → satisfy) ([#21629](facebook/react#21629)) //<niexq>// - **[b610fec00](facebook/react@b610fec00 )**: fix comments: expiration time -> lanes ([#21551](facebook/react#21551)) //<Shannon Feng>// - **[cc4d24ab0](facebook/react@cc4d24ab0 )**: [Fizz] Always call flush() if it exists ([#21625](facebook/react#21625)) //<Dan Abramov>// - **[e0d9b2899](facebook/react@e0d9b2899 )**: [Fizz] Minor Fixes for Warning Parity ([#21618](facebook/react#21618)) //<Sebastian Markbåge>// - **[1b7b3592f](facebook/react@1b7b3592f )**: [Fizz] Implement Component Stacks in DEV for warnings ([#21610](facebook/react#21610)) //<Sebastian Markbåge>// - **[39f007489](facebook/react@39f007489 )**: Make enableSuspenseLayoutEffectSemantics static for www ([#21617](facebook/react#21617)) //<Brian Vaughn>// - **[8f3794276](facebook/react@8f3794276 )**: Prepare semver (`latest`) releases in CI ([#21615](facebook/react#21615)) //<Andrew Clark>// - **[8b4201535](facebook/react@8b4201535 )**: Devtools: add feature to trigger an error boundary ([#21583](facebook/react#21583)) //<Bao Pham>// - **[154a8cf32](facebook/react@154a8cf32 )**: Fix reference to wrong variable //<Andrew Clark>// - **[6736a38b9](facebook/react@6736a38b9 )**: Add single source of truth for package versions ([#21608](facebook/react#21608)) //<Andrew Clark>// - **[86715efa2](facebook/react@86715efa2 )**: Resolve the true entry point during tests ([#21505](facebook/react#21505)) //<Sebastian Markbåge>// - **[a8a4742f1](facebook/react@a8a4742f1 )**: Convert ES6/TypeScript/CoffeeScript Tests to createRoot + act ([#21598](facebook/react#21598)) //<Sebastian Markbåge>// - **[1d3558965](facebook/react@1d3558965 )**: Disable deferRenderPhaseUpdateToNextBatch by default ([#21605](facebook/react#21605)) //<Sebastian Markbåge>// - **[a8964649b](facebook/react@a8964649b )**: Delete an unused field ([#21415](facebook/react#21415)) //<okmttdhr>// - **[76f85b3e5](facebook/react@76f85b3e5 )**: Expose Fizz in stable builds ([#21602](facebook/react#21602)) //<Dan Abramov>// - **[e16d61c30](facebook/react@e16d61c30 )**: [Offscreen] Mount/unmount layout effects ([#21386](facebook/react#21386)) //<Andrew Clark>// - **[63091939b](facebook/react@63091939b )**: OSS feature flag updates ([#21597](facebook/react#21597)) //<Brian Vaughn>// - **[efbd69b27](facebook/react@efbd69b27 )**: Define global __WWW__ = true flag during www tests ([#21504](facebook/react#21504)) //<Sebastian Markbåge>// - **[28625c6f4](facebook/react@28625c6f4 )**: Disable strict effects for legacy roots (again) ([#21591](facebook/react#21591)) //<Brian Vaughn>// - **[3c2341416](facebook/react@3c2341416 )**: Update jest to v26 ([#21574](facebook/react#21574)) //<Sebastian Silbermann>// - **[0d493dcda](facebook/react@0d493dcda )**: Removed _debugID field from Fiber - Issue #21558 ([#21570](facebook/react#21570)) //<Pulkit Sharma>// - **[7841d0695](facebook/react@7841d0695 )**: Enable the updater-tracking feature flag in more builds ([#21567](facebook/react#21567)) //<Brian Vaughn>// - **[6405efc36](facebook/react@6405efc36 )**: Enabled Profiling feature flags for OSS release ([#21565](facebook/react#21565)) //<Brian Vaughn>// Changelog: [General][Changed] - React Native sync for revisions 2d8d133...0eea577 jest_e2e[run_all_tests] Reviewed By: bvaughn Differential Revision: D28932083 fbshipit-source-id: 012c1bfb857ed59d7283334d633f1cce8ec50360
This is a rebase of #18974 in the hopes that it would get merged.
Add support for templates published as scoped packages. Partially fixes #18973.
Test Plan
Published a template as a scoped package and installed it.
Related PRs
Documentation says nothing regarding lack of support for scoped packages, so intuitively they should work. This pull-request just makes that intuitive assumption true, so I think no changes on documentation are needed.
Release Notes
[CLI] [BUGFIX] [local-cli/generator/templates.js] - Support for scoped npm packages as templates