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

Scaffold multi-package workspace. #6850

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Scaffold multi-package workspace. #6850

merged 1 commit into from
Jul 3, 2024

Conversation

anthonyshew
Copy link
Contributor

@anthonyshew anthonyshew commented Jun 19, 2024

Description

Create the structure for a multi-package workspace for the Node.js website.

Core maintainers have expressed a desire for re-using code from the website in other places. These changes allows for those publishable packages to be created, since they can now be broken out to their own separate packages.

Unfortunately, this involves a large-scale cutover from the repository's previous structure, invalidating PR's that come from current main. I've received maintainer approval for my disruptiveness. 🙏

Fortunately, those outstanding PRs can be fairly easily re-aligned with this PR, since I've made near-zero changes as far as the source code is concerned. This PR updates tooling, rather than functional code. (Prettier updated ~10 files that I saw at one point, but I don't believe this came from the changes I've made. I think the source code was somehow out-of-sync with the Prettier configuration.) If you'd like your PR to meet back up with this one, you should be able to move them from their current location to ./apps/site/<your-path>.

Validation

  • One thing I can say I certainly don't know about in this repo is crowdin.yml. I'm not familiar with that tool so I don't know if it makes sense to move into the apps/site directory or if it needs to stay in the root.
  • I've tried to find breakages and configuration differences from main to here and have satisfied my initial attempts to break myself.
  • We can name the directory for the site something other than site, as desired. Just let me know!
  • I'll need some help making sure that CI has been re-aligned properly, both in GHA and in Vercel (cc @ovflowd in particular for Vercel, I believe).

Anthony's Checklist

  • Does the site build locally?
  • Does husky pass?
  • Do lints pass locally?
  • Does lint-staged work?
  • Do test pass locally?
  • Do commands in the root of the repository run?
  • Do the expected turbo tasks run when invoked?
  • Update repo's meta-documentation for contributors
  • Does internationalization work as expected?
  • Does Vercel build?
  • Re-align GHA checks

Copy link

vercel bot commented Jun 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ❌ Failed (Inspect) Jul 3, 2024 9:13pm

# See https://turbo.build/repo/docs/reference/command-line-reference/run#--force
run: echo "turbo_args=--force=true --cache-dir=.turbo/cache" >> "$GITHUB_OUTPUT"
Copy link
Contributor Author

@anthonyshew anthonyshew Jun 23, 2024

Choose a reason for hiding this comment

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

This is no longer needed as of Turborepo 2, since it's the default.

@bmuenzenmeyer
Copy link
Collaborator

bmuenzenmeyer commented Jun 24, 2024

I looked into crowdin renames - this was the best I could find so far.

https://community.crowdin.com/t/change-source-file-name-without-loosing-progress/7594/3

I will ask in our Slack channel with Crowdin to confirm or supply something more detailed

@Andrulko
Copy link
Contributor

One thing I can say I certainly don't know about in this repo is crowdin.yml. I'm not familiar with that tool so I don't know if it makes sense to move into the apps/site directory or if it needs to stay in the root.

Please keep this file in the root of the branch, it is used by Crowdin <-> GitHub connector:
https://support.crowdin.com/github-integration/

It would be required to update paths to EN files to be able to sync sources and translations:
https://github.com/nodejs/nodejs.org/blob/main/crowdin.yml

Please pause Crowdin <-> GitHub sync before merging the PR, then after it's merged Crowdin team will help to adapt the existing Crowdin project and configuration file to the new structure.

@anthonyshew
Copy link
Contributor Author

Thanks, @Andrulko! I've remapped those paths now, if you'd like to check them to confirm I've done so correctly.

@Andrulko
Copy link
Contributor

@anthonyshew Just to make sure that everything would work fine, let's insert / at the beginning of path. Please use the configuration below:

commit_message: 'chore: synced translations from crowdin [skip ci]'
append_commit_message: false
pull_request_title: '[automated]: crowdin sync'
pull_request_labels:
  - 'github_actions:pull-request'
files:
  - source: /apps/site/pages/en/**/*.md
    translation: /apps/site/pages/%two_letters_code%/**/%original_file_name%
    content_segmentation: 0
    ignore:
      - /apps/site/pages/en/blog/**/*.md
      - /apps/site/pages/en/learn/**/*.md
      - /apps/site/pages/en/download/index.md
      - /apps/site/pages/en/download/current.md
    languages_mapping:
      two_letters_code:
        es-ES: es
        pt-BR: pt-br
        zh-CN: zh-cn
        zh-TW: zh-tw
  - source: /apps/site/pages/en/**/*.mdx
    translation: /apps/site/pages/%two_letters_code%/**/%original_file_name%
    content_segmentation: 0
    ignore:
      - /apps/site/pages/en/blog/**/*.mdx
      - /apps/site/pages/en/learn/**/*.mdx
    languages_mapping:
      two_letters_code:
        es-ES: es
        pt-BR: pt-br
        zh-CN: zh-cn
        zh-TW: zh-tw
  - source: /apps/site/i18n/locales/en.json
    translation: /apps/site/i18n/locales/%two_letters_code%.json
    languages_mapping:
      two_letters_code:
        es-ES: es
        pt-BR: pt-br
        zh-CN: zh-cn
        zh-TW: zh-tw

Everything else looks good! Before you merge this branch, please pause GitHub sync in Crowdin (otherwise, it will upload new /apps/site folders along with files). I will help to move the existing files in Crowdin project to the new path and then we can safely resume the sync. No new files will be uploaded in this case 👌

@anthonyshew
Copy link
Contributor Author

Updated with that. Thank you!

@bmuenzenmeyer

This comment was marked as resolved.

package.json Outdated Show resolved Hide resolved
Copy link

Running Lighthouse audit...

@anthonyshew
Copy link
Contributor Author

I think I've gotten things as close as I can at this point without giving it another real run:

  • Squashed everything down to one commit for rebasing purposes
  • Hand-tested the blog post generation and Orama syncing scripts
  • Ran tests and lints on my fork to see them green
  • Updated the working directory for the Storybook Action

With that said, I think I'm ready to get labelled for another run again. 🙏

@ovflowd
Copy link
Member

ovflowd commented Jul 3, 2024

Thanks, @anthonyshew! Your work and effort is extremely appreciated!

I believe CI will fail in this branch but we can proceed with a merge if you are confident with things.

cc @nodejs/nodejs-website and @bmuenzenmeyer for a final PR review run 👀

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Jul 3, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Jul 3, 2024
@anthonyshew
Copy link
Contributor Author

Looking through the CI errors on this run, these are all what I would expect to see from the changes I've made running against the Actions from main. I think I've accounted for all the failures I'm seeing but am prepared to be pseudo-on-call and quickly fix-forward as needed.

The Root Directory on Vercel will need to be updated to apps/site, which would earn these checks another green check mark if re-deployed with that setting. Even if not, you'd want to update that setting prior to merging, so that you get a good production deployment.

@ovflowd
Copy link
Member

ovflowd commented Jul 3, 2024

Gotcha! Ill give another review in a few minutes and Im fine with bearing the responsibilities of the merge.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

It looks good to me. @bmuenzenmeyer note that Crowdin might break? But that's an issue to verify as a follow-up. I'm merging this now and going to observe what happens on the main branch

@ovflowd ovflowd merged commit 3a455b2 into nodejs:main Jul 3, 2024
7 of 11 checks passed
@anthonyshew anthonyshew mentioned this pull request Jul 4, 2024
5 tasks
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Jul 17, 2024
PR-URL: #53762
Refs: nodejs/nodejs.org#6850
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
PR-URL: nodejs#53762
Refs: nodejs/nodejs.org#6850
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit to nodejs/node that referenced this pull request Jul 28, 2024
PR-URL: #53762
Refs: nodejs/nodejs.org#6850
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
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.

None yet

4 participants