consul
consul copied to clipboard
hashicorp
ui: Move nvmrc to the root of the workspace
Description
Back in https://github.com/hashicorp/consul/pull/8994 we moved our "in repo" UI repo to use a workspace'd structure yet did not get the opportunity to finish that migration up. Therefore there are several config files that should probably be in the root of the workspace rather than the consul-ui folder, now that we have multiple packages that should all use the same environment/config. Other files (and other considerations) have been left for a future PR, but I've noticed that git hook linting sometimes doesn't seem to be running on PRs (the nvmrc file is currently only in consul-ui, not our other packages), so I've moved this nvmrc file to the root of the workspace which should help that, as well as helping newer contributors to the codebase.
Note: I was going to add a quick nvm use to the deps Makefile target, but I've noticed some folks are using asdf so I didn't want to do too much here if we are to potentially consider moving to using that to pin our node version.
PR Checklist
- [ ] updated test coverage
- [ ] external facing docs updated
- [x] not a security concern
FWIW I'm using volta for managing node versions and not nvm or asdf. This feels like a generic setup for the entire monorepo setup for the frontend codebase—would it make sense to tackle this all at once in one go?
FWIW I'm using volta for managing node versions and not nvm or asdf.
I guess everyone has their favourites I just went with what other HashiCorp repos were using at the time to stick with common tooling:
https://github.com/hashicorp/nomad/blob/main/ui/.nvmrc https://github.com/hashicorp/vault/blob/main/ui/.nvmrc
This feels like a generic setup for the entire monorepo setup for the frontend codebase—would it make sense to tackle this all at once in one go?
Not quite sure what you are referring to/asking here. This makes the config available to the entire monorepo. If you mean is there any other left over work to finish moving everything to monorepo, then yes there probably is. But this one thing was potentially causing node based githooks not to run though, which I'd noticed personally so I wanted to get at least this done if possible.
Anyway let me know what you meant there as I might be misunderstanding.
@LevelbossMike I hadn't heard of Volta before. Looks handy! Are you looking for some additional changes to ensure that Volta is also supported? Or maybe a better question to ask here is, "what would need to be added to this in order for Volta to also be able to take advantage of these features?"
@evrowe @johncowen I was trying to point out that other more recent options than nvm exist and that coding against nvm might not be the ideal solution for all scenarios. Volta, for example, would be an alternative that adds its configuration into package.json directly and supports pinning yarn on top of node versions—but that would also mean to tell everyone to use volta. I am uncertain if that's what we want, either.
The second point I was trying to make, relates to something pointed out by John: There are other files that relate to the yet to complete monorepo setup that we might also want to consider putting into the root of the monorepo, linting related files for example. And I wondered if we are aware of this, and we are already updating the codebase, if it wouldn't make sense to tackle this all-in-one go.
Having said that, this was more of a general idea/comment and shouldn't be blocking this PR if this is an urgent fix.
FYI, this was just something I'd noticed with our current setup. As we'd never properly finished the move to workspaces in https://github.com/hashicorp/consul/pull/8994, I quite often go to commit things in other non consul-ui packages, and as the correct version of node isn't installed I don't think the lint git hook has been running.
The version of node should be pinned for our entire project not just a single sub package, so the change here would make sense even if it was just something I'd noticed for myself.
Longer term, more than happy to look at using a different tool for pinning deps, but I think we should choose one and go with it and then IMO ideally make should hide whatever that tool is from the end user/engineer (but allow people more aware of our toolchain to choose where in that chain they want to begin). Our entire team/org/company uses make.
That being said, right now I don't want to get into a discussion around which tool to use, just move a file that happens to be in the wrong place for the tool that we currently use. As it stands now, I've currently moved the file locally just not commited/merged as yet. If it's a big deal happy to just leave that as is.
@LevelbossMike as far as I'm aware there is nothing ongoing to finish up the move to workspace work, but we should definitely do that. As you pointed out there are other things we should also much such as linting config etc. But me personally, I don't think it's important to block this PR until we get around that other work, but would be good to chat through what else we want to move to the workspace root.
I agree, let's put a pin in this discussion and talk about tooling choices moving forward outside the context of this particular PR. These changes should be good to commit as-is.
@evrowe did you mean to approve this with that last comment?
These changes should be good to commit as-is.