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

[expo-cli][xdl] Install templates from npm registry #217

Merged
merged 8 commits into from Nov 27, 2018

Conversation

@fson
Copy link
Member

commented Nov 26, 2018

expo init templates are now published as npm packages. This way
third-party templates can be used with expo init --template <name>.

This also adds the capability to have multiple versions of a template
and tag them, for example to support templates for different SDK
versions.

Also the dependencies are now installed using npm or Yarn. The choice
between Yarn and npm is made as follows.

  • If explicitly specified with --npm or --yarn option to expo init,
    always try to use that one.
  • If Yarn is not installed, always try to use npm.
  • If Yarn is installed, ask if the user wants to use it. We do this because
    if we use Yarn, and the user then uses npm to add packages, they end up
    with a node_modules tree that is missing packages.

The --template argument passed to expo init can be anything
supported by npm install: e.g. a package from npm registry, a git
repo, tarball or local file.

Examples:

  • expo init --template expo-template-tabs: use the latest version of
    the tab navigation templates.
  • expo init --template tabs: same as the above, the blank and tabs
    aliases are provided for backwards compatibility.
  • expo init --template expo-template-blank@sdk-31: use the sdk-31
    tagged version of the blank template.
  • expo init --template https://github.com/expo/new-project-template.git:
    use a git repository as a template.
[expo-cli][xdl] Install templates from npm registry
`expo init` templates are now published as npm packages. This way
third-party templates can be used with `expo init --template <name>`.

This also adds the capability to have multiple versions of a template
and tag them, for example to support templates for different SDK
versions.

Also the dependencies are now installed using npm or Yarn. The choice
between Yarn and npm is made as follows.
- If explicitly specified with `--npm` or `--yarn` option to `expo init`,
  always try to use that one.
- If Yarn is not installed, always try to use npm.
- If Yarn is installed, ask if the user wants to use it. We do this because
  if we use Yarn, and the user then uses npm to add packages, they end up
  with a `node_modules` tree that is missing packages.

The `--template` argument passed to `expo init` can be anything
supported by `npm install`: e.g. a package from npm registry, a git
repo, tarball or local file.

Examples:
- `expo init --template expo-template-tabs`: use the latest version of
  the tab navigation templates.
- `expo init --template tabs`: same as the above, the `blank` and tabs`
  aliases are provided for backwards compatibility.
- `expo init --template expo-template-blank@sdk-31`: use the sdk-31
  tagged version of the blank template.
- `expo init --template https://github.com/expo/new-project-template.git`:
  use a git repository as a template.

@fson fson requested review from brentvatne and dsokal Nov 26, 2018

@fson fson added the in progress label Nov 26, 2018

@fson fson force-pushed the @fson/expo-init-templates branch from f60201d to d365c46 Nov 26, 2018

@brentvatne

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

@fson - :shipit:

@dsokal
Copy link
Member

left a comment

Few comments inline. Also, how did you publish expo-template-tabs and expo-template-blank? Where are these packages?

packages/expo-cli/src/commands/init.js Show resolved Hide resolved
.toString()
.trim();
if (!semver.valid(version)) {
console.log(`${version} aint valid`);

This comment has been minimized.

Copy link
@dsokal

dsokal Nov 27, 2018

Member

Hmm... I guess we need a better log here :P

This comment has been minimized.

Copy link
@fson

fson Nov 27, 2018

Author Member

Oops 🤡

});
return answer.useYarn;
} catch (e) {
console.log(e);

This comment has been minimized.

Copy link
@dsokal

dsokal Nov 27, 2018

Member

I guess we don't want to console.log this error, it doesn't mean anything to a user who doesn't have yarn installed:

Error: Command failed: yarnpkg --version
/bin/sh: yarnpkg: command not found

    at checkExecSyncError (child_process.js:602:13)
    at execSync (child_process.js:642:13)

This comment has been minimized.

Copy link
@fson

fson Nov 27, 2018

Author Member

Uh, yeah, sorry about that. Thanks for finding these 🙈

// pacote adds these, but we don't want them in the package.json of the project.
delete packageJson._resolved;
delete packageJson._integrity;
delete packageJson._from;

This comment has been minimized.

Copy link
@dsokal

dsokal Nov 27, 2018

Member

I don't like using delete, i.e. mutating JS objects. I believe it would be better to use _.omit from lodash. It could also help to write these 8 lines in just one line of code.

This comment has been minimized.

Copy link
@fson

fson Nov 27, 2018

Author Member

You're right to be careful about mutation, since non-local mutation can cause trouble (e.g. if I was mutating an object passed to this function as an argument), but in this case it's local mutation, which is completely fine.

I don't use lodash for things that can be done easily with plain JS. _.omit will also be harder to correctly type with TypeScript or Flow.

However, since you pointed this out, I've now changed the code to defensively clone the object before mutating it to make the intent (mutating only locally) clearer.

This comment has been minimized.

Copy link
@dsokal

dsokal Nov 27, 2018

Member

I totally get your point about not using lodash if there is a similar feature in plain JS. I try to follow this rule as well, but if there is something which helps me to reduce the code complexity (by having fewer lines of code) and helps to be more explicit then I say you should use lodash for such things.

Also, lodash works well with Typescript. I don't know if it works well with Flow, but I wouldn't assume it doesn't.

This comment has been minimized.

Copy link
@fson

fson Nov 27, 2018

Author Member

My thinking with regards to types was that using delete would help code like this produce a type error:

let data = { a: 1, b: 2 };
delete data.a;
console.log(data.a + data.b);

However, it turns out this code passes the type check in both TypeScript and Flow (bummer!) so my argument is moot. 😊

I think we're in agreement about lodash. But, in this particular case fewer lines of code doesn't bring much value (I actually prefer having these properties on separate lines, this gives better diffs in case fields get added/removed) and I think delete here is also quite explicit.

@@ -139,6 +106,28 @@ function isNonExistentOrEmptyDir(dir) {
}
}

async function shouldUseYarnAsync() {
try {
let version = execSync('yarnpkg --version')

This comment has been minimized.

Copy link
@dsokal

dsokal Nov 27, 2018

Member

I would prefer to use async version of exec - it doesn't cost anything and it's always better to avoid using long-running synchronous functions in JS. I think it encourages a programmer to reuse code.

This comment has been minimized.

Copy link
@fson

fson Nov 27, 2018

Author Member

👍

// Rename `gitignore` because npm ignores files named `.gitignore` when publishing.
// See: https://github.com/npm/npm/issues/1862
try {
fs.moveSync(path.join(projectRoot, 'gitignore'), path.join(projectRoot, '.gitignore'));

This comment has been minimized.

Copy link
@dsokal

dsokal Nov 27, 2018

Member

fs.moveSync -> await fs.move

This comment has been minimized.

Copy link
@fson

fson Nov 27, 2018

Author Member

👍

@fson

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

Thanks for the review @dsokal! The default templates are still in universe (a related PR at expo/universe#3313), but should be moved to the expo/expo repo at some point.

@dsokal

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Np. I didn't see that PR, sorry (I left two comments there as well).
Could you please reply to my latest comment in this PR before hitting "Merge" button? Thanks!

@fson fson merged commit 727cb92 into master Nov 27, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@fson fson deleted the @fson/expo-init-templates branch Nov 27, 2018

@fson fson removed the in progress label Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.