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

Handle the tags/admin/ client as an npm workspace at /tag-admin/ #2430

Merged
merged 8 commits into from
Feb 16, 2022

Conversation

eemeli
Copy link
Member

@eemeli eemeli commented Feb 15, 2022

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 favour npm ci over npm 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:

"@babel/cli": "^7.13.10",
"@babel/plugin-proposal-class-properties": "^7.13.0",
"babel-plugin-transform-es2015-modules-commonjs": "^6.26.2",
"jsdom": "^11.12.0",
"react-test-renderer": "^16.14.0",
"request": "^2.88.2",
"style-loader": "^0.19.1",

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.

Import paths are modified to use relative, fully qualified file names.
Consistently call 'npm ci' and 'npm run build' where appropriate
@eemeli eemeli requested a review from mathjazz February 15, 2022 14:36
Copy link
Collaborator

@mathjazz mathjazz left a 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.

.github/workflows/non-frontend-js.yml Outdated Show resolved Hide resolved
- name: prettier
run: ./node_modules/.bin/prettier --check '**/frontend/**/*.{js,ts,tsx,css}' '**/pontoon/**/*.{js,css}' '**/tests/**/*.{js,css}'
run: npm run check-prettier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)

@eemeli
Copy link
Member Author

eemeli commented Feb 15, 2022

I haven't been able to figure out why did the package-lock.json grow so big, though.

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.

Is the plan to make frontend a workspace, too?

Yes, eventually. That would be a part of moving its package management from yarn to npm.

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.

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?

@Pike
Copy link
Collaborator

Pike commented Feb 15, 2022

Would this be a good time to also yarn the tags UI instead of npm?

Using one and the other every now and then bothered me for a long time.

@eemeli
Copy link
Member Author

eemeli commented Feb 16, 2022

Would this be a good time to also yarn the tags UI instead of npm?

Yes, but no? I'm actually working towards using npm also for the frontend.

@mathjazz
Copy link
Collaborator

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?

# Tag Admin

This is an [npm workspace](https://docs.npmjs.com/cli/v7/using-npm/workspaces)
responsible for the tag management widget of the project administration page.

@mathjazz
Copy link
Collaborator

Could you please rebase?

Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/

@eemeli eemeli merged commit d26c914 into mozilla:master Feb 16, 2022
@eemeli eemeli deleted the tagadmin-workspace branch February 16, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants