-
Notifications
You must be signed in to change notification settings - Fork 530
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
Handle the tags/admin/
client as an npm workspace at /tag-admin/
#2430
Conversation
Import paths are modified to use relative, fully qualified file names.
Consistently call 'npm ci' and 'npm run build' where appropriate
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.
The new file & folder structure makes so much more sense and I'm already a fan of npm workspaces. :) Thanks!
I haven't been able to figure out why did the package-lock.json
grow so big, though.
Is the plan to make frontend
a workspace, too?
Might be a good idea to add a README.md file to the tag-admin
folder, with a very short description of what it does and a linked explanation it's a npm workspace.
- name: prettier | ||
run: ./node_modules/.bin/prettier --check '**/frontend/**/*.{js,ts,tsx,css}' '**/pontoon/**/*.{js,css}' '**/tests/**/*.{js,css}' | ||
run: npm run check-prettier |
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.
Nice :)
It's because the lockfile format is here updated to v2, which needs to duplicate all of the entries for backwards compatibility. v3 will eventually shrink it by half. The actual count of discrete items is reduced, though.
Yes, eventually. That would be a part of moving its package management from yarn to npm.
Is there an existing description of the tag admin tool available somewhere that could be used for this? If not, could you write a few words about it? |
Would this be a good time to also Using one and the other every now and then bothered me for a long time. |
Yes, but no? I'm actually working towards using |
|
Could you please rebase? |
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.
\o/
This is a part of solving #2423.
As a first step, this PR separates the tag-admin front-end sources, build and dependencies into a new top-level directory
/tag-admin/
. The result of the build should not change at all, but the sources and configs are now more clear.This introduces a dependency on npm v7 or later, as earlier versions did not support workspaces. Build & CI tasks are updated accordingly, in a few cases switching to use
node-version: 16
, as that includes a sufficient up-to-date npm. Some minor cleanup is done there also to favournpm ci
overnpm install
, where appropriate.The tag-admin contents or dependencies are left exactly as before, though a few files are renamed and the JS
import
statements are adjusted to use relative, fully qualified paths.The following packages are removed from dev dependencies, as no actual usage for them could be identified:
NOTE: Because of the npm workspace config change, you may need to run
rm -rf node_modules && npm install
at the repo root in order to get the right topography after this is merged.