jpmorganchase / modular

A modular front end development framework
https://modular.js.org/
Apache License 2.0
619 stars 67 forks source link

Running "npx create-modular-react-app" installs create-react-app globally #46

Closed threepointone closed 3 years ago

threepointone commented 4 years ago

Why tho? lol. Does craco do this behind the scenes or something?

Can confirm by running "yarn global list". Remove it if it's there, run npx create-modular-react-app, and see it again.

Also it might be installing create-modular-react-app globally too, but I've not been able to reproduce it after seeing it once.

Lost some time today wondering why I was generating older version of the app booo.

threepointone commented 4 years ago

Ok, so this is what happens. We internally use yarn create react-app ..., and it looks like yarn create installs the package globally as a feature. Mad I didn't know this, and it's very, very annoying. We should probably move to npx for this command.

NMinhNguyen commented 4 years ago

It also says

or update the package to the latest version if it already exists

threepointone commented 4 years ago

Also terrible.

NMinhNguyen commented 4 years ago

Is that not what npx would do? Get the latest version?

threepointone commented 4 years ago

It doesn't install it globally. (Or to be specific, it doesn't leave it behind after it's done executing.)

NMinhNguyen commented 4 years ago

Sorry if I wasn't clear but I was more commenting on this:

Lost some time today wondering why I was generating older version of the app booo.

That made me think that the latest version would solve this particular problem?

threepointone commented 4 years ago

But it wasn't pulling the latest version, that's why I filed the bug. Suspicious.

sebinsua commented 4 years ago

I've marked this as 'good first issue'.

Basically, all that we need to do is to find all of the cases in the code and documentation where we use yarn create modular-react-app ${appName} or execa('yarn', ['create', 'modular-react-app', appName]) and replace these with calls to npx (which is more modern and easy-to-remember).

threepointone commented 4 years ago

Unfortunately it's not as simple as replacing yarn commands with npx, specifically because those changes won't be reflected in yarn.lock, etc. I think the correct solution to this will be to move to npx fully, but that's a while away (of note, the npm 7 launched today). Removing the good first issue tag.

NMinhNguyen commented 4 years ago

Unfortunately it's not as simple as replacing yarn commands with npx, specifically because those changes won't be reflected in yarn.lock, etc.

Could you elaborate on this? The yarn create commands don't have anything to do with yarn.lock.

Separately to this is the fact we spent over an hour as a team going over these issues and agreeing on labels and priorities during last week's sprint planning, I don't think that labels should be removed or priorities changed without a team discussion/agreement.

susanafcosta commented 4 years ago

Unfortunately it's not as simple as replacing yarn commands with npx, specifically because those changes won't be reflected in yarn.lock, etc.

Could you elaborate on this? The yarn create commands don't have anything to do with yarn.lock.

Separately to this is the fact we spent over an hour as a team going over these issues and agreeing on labels and priorities during last week's sprint planning, I don't think that labels should be removed or priorities changed without a team discussion/agreement.

Agreed with both statements! All issues should be discussed, agreed and prioritised by the team.

threepointone commented 4 years ago

Could you elaborate on this? The yarn create commands don't have anything to do with yarn.lock

Make a new repository, with workspaces. (use modular if you'd like). Then under packages/, run yarn create react-app app1. You'll see that it changes the root yarn.lock file. This is preferred behaviour. Now run npx create-react-app app2, you'll see that it doesn't, and makes a node_modules folder under app2 (and a package-lock.json), and won't share dependencies with the rest of the repo. This isn't preferred behaviour. Hence my point, that it isn't as trivial as replacing the internal commands with npx, if we still have to use yarn workspaces (and yarn lockfiles). So it isn't a "great first issue", and I've corrected that, with an explanation.

Separately. I haven't changed the priority (noting that the prioritisation happened on my week off, but ok.) But I'm also not going to hold every single change I want to make for planning meetings. We work asynchronously on this project and related stuff, and if I had to get the whole team's buy in for every small change, we wouldn't be able to scale. It's not like the whole team's input is taken for prioritising every issue on any of the side projects either. I'm happy to be corrected on individual items and discuss here (like this very thread), which is why I leave notes for any changes I make. Considering I'm dong the bulk of the work in this repo, I don't think it's unreasonable to have the freedom to make the changes I'm proposing.

NMinhNguyen commented 4 years ago

Separately. I haven't changed the priority

I was speaking generally, but here's a concrete example: https://github.com/jpmorganchase/modular/issues/123#event-3866837624. If something isn't clear, you can use the internal chatroom to ask the team instead of removing a priority that was collectively assigned. In both these cases, a label was removed (good first issue and medium priority, respectively).

Could you elaborate on this? The yarn create commands don't have anything to do with yarn.lock

Make a new repository, with workspaces. (use modular if you'd like). Then under packages/, run yarn create react-app app1. You'll see that it changes the root yarn.lock file. This is preferred behaviour. Now run npx create-react-app app2, you'll see that it doesn't, and makes a node_modules folder under app2 (and a package-lock.json), and won't share dependencies with the rest of the repo. This isn't preferred behaviour. Hence my point, that it isn't as trivial as replacing the internal commands with npx, if we still have to use yarn workspaces (and yarn lockfiles). So it isn't a "great first issue", and I've corrected that, with an explanation.

Thanks for the detailed explanation, the original one wasn't sufficiently clear/obvious - if I run npx create-react-app in isolation, I do get yarn.lock and .git initialised correctly because I have Yarn installed on my machine: https://github.com/facebook/create-react-app/blob/50368252ed40b2838373c4007fc841ac17b950ef/packages/create-react-app/createReactApp.js#L274

Separately. I haven't changed the priority (noting that the prioritisation happened on my week off, but ok.) But I'm also not going to hold every single change I want to make for planning meetings. We work asynchronously on this project and related stuff, and if I had to get the whole team's buy in for every small change, we wouldn't be able to scale. It's not like the whole team's input is taken for prioritising every issue on any of the side projects either. I'm happy to be corrected on individual items and discuss here (like this very thread), which is why I leave notes for any changes I make. Considering I'm dong the bulk of the work in this repo, I don't think it's unreasonable to have the freedom to make the changes I'm proposing.

All of the work we do, especially the side projects such as an internal cache implementation, is at the very least meant to be brought up initially during sprint planning to ensure everyone is on board with what's being proposed and to make sure that each sprint's scope is defined (this helps avoid scope creep). Side conversations do happen, such as me discussing caching changes/features with @sebinsua, but the requirements never just appear out of the blue, and everyone does their best to communicate changes either during Zoom calls or via chat.

threepointone commented 4 years ago

I’m not going to have a separate side conversation about an issue when you’re already getting notifications for every comment here and we can have a conversation right here, like we’re doing right now.

NMinhNguyen commented 4 years ago

Do you not see any issue with changing existing priorities/labels, without first having a discussion about it? To me, discussing any change prior to making it sounds like the courteous thing to do. Or would you prefer for others to go back to issues and reinstate labels that have been removed? We used to have daily standups which acted as a regular touchpoint to ensure everyone is up to date (which is useful, especially given the current remote working times), but if memory serves my correctly, you asked for them to be every other day instead. The key thing here is transparency. We are a pretty small team, it shouldn't be that difficult to keep everyone in sync, and for mutual agreement to be reached.

NMinhNguyen commented 4 years ago

Make a new repository, with workspaces. (use modular if you'd like). Then under packages/, run yarn create react-app app1. You'll see that it changes the root yarn.lock file. This is preferred behaviour. Now run npx create-react-app app2, you'll see that it doesn't, and makes a node_modules folder under app2 (and a package-lock.json), and won't share dependencies with the rest of the repo. This isn't preferred behaviour. Hence my point, that it isn't as trivial as replacing the internal commands with npx, if we still have to use yarn workspaces (and yarn lockfiles). So it isn't a "great first issue", and I've corrected that, with an explanation.

So I actually just created a brand new folder, did yarn init -y, added "private": true and "workspaces": "packages/*", cded into packages and ran npx create-react-app app and it did not create package-lock.json. It actually updated the root-level yarn.lock (as well as creating a yarn.lock file in the app folder due to https://github.com/facebook/create-react-app/blob/50368252ed40b2838373c4007fc841ac17b950ef/packages/create-react-app/createReactApp.js#L330-L333, but that should be fixable within CRA, and isn't a big issue to just delete that file or even have it lying around as it won't be picked up by Yarn anyway).

threepointone commented 4 years ago

Noted, you're right.

threepointone commented 3 years ago

We're sticking with yarn for the foreseeable future, closing this.