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

Remove duplicate yarn.lock and move algolia script #324

Merged
merged 7 commits into from
Feb 22, 2022

Conversation

kachkaev
Copy link
Contributor

@kachkaev kachkaev commented Feb 17, 2022

🌟 What is the purpose of this PR?

This PR removes duplicate yarn.lock from .github/workflows/algolia/yarn.lock and thus consolidates monorepo dependencies. This simplifies further maintenance of our codebase. A similar PR can be found in blockprotocol/blockprotocol#169

⚠️ Known issues

  • Not sure if files in [root]/scripts/* are property covered by all linters at the moment, but at least it did not become worse.

  • I could not test the script because I don’t have algolia keys.

🔍 What does this change?

🔗 Related links

🛡 What tests cover this?

  • None 🙈

❓ How to test this?

  1. Run yarn exe scripts/sync-algolia-index.ts

  2. Observe this if you don’t know Algolia credentials
    Screenshot 2022-02-17 at 17 38 20

    OR Confirm that you get Algolia index updated. ortherwise.

@github-actions github-actions bot added area/infra Relates to version control, CI, CD or IaC (area) area/deps Relates to third-party dependencies (area) labels Feb 17, 2022
Comment on lines +22 to +24
- name: Reduce the number of dependencies to install by hiding workspaces
run: |
npx replace '"workspaces":' '"ignored-workspaces":' package.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This trick should reduce yarn install time by skipping all dependencies in all workspaces. We might be able to do something more ‘official’ after upgrading to Yarn Berry — docs: https://yarnpkg.com/cli/workspaces/focus

Focus-mode does not work for root package.json in Yarn 1 AFAIU.

Comment on lines 22 to +24
"dev:backend:search-loader": "yarn workspace @hashintel/hash-search-loader dev",
"dev:frontend": "yarn workspace @hashintel/hash-frontend dev",
"exe": "ts-node --require dotenv-flow/config --transpile-only",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar setup as in the blockprotocol repo. I just did not add the swc transpiler to make our dependency tree lighter. We can introduce swc when we have more scripts and can therefore benefit more from performance.

@@ -56,9 +57,13 @@
"node-fetch": "^2.6.7"
},
"devDependencies": {
"algoliasearch": "4.12.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was in .github/workflows/algolia/package.json

Comment on lines +54 to 56
objectId: undefined;
content: string;
objectID: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve been getting a type error here because of what we do on line 68 (objectId: undefined,). @akash-joshi what’s the difference between objectId and objectID? I want to keep logic changes minimal, but happy to add a comment or tweak types a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

objectId was an old key holding auto-generated objectIDs for each Algolia record (#37). This is now deprecated by the path-based objectID generation PR (#49), hence the old objectIds are set to undefined.

Comment on lines +102 to +112
const env = envalid.cleanEnv(process.env, {
ALGOLIA_PROJECT: envalid.str({
desc: "Algolia app id",
example: "A1B2C3D4C5D6",
docs: "https://www.algolia.com/doc/api-client/getting-started/instantiate-client-index/javascript/?client=javascript",
}),
ALGOLIA_WRITE_KEY: envalid.str({
desc: "Algolia app API key with write permissions (32-char HEX)",
docs: "https://www.algolia.com/doc/api-client/getting-started/instantiate-client-index/javascript/?client=javascript",
}),
});
Copy link
Contributor Author

@kachkaev kachkaev Feb 17, 2022

Choose a reason for hiding this comment

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

@kachkaev kachkaev marked this pull request as ready for review February 17, 2022 17:40
akash-joshi
akash-joshi previously approved these changes Feb 21, 2022
Copy link
Contributor

@akash-joshi akash-joshi left a comment

Choose a reason for hiding this comment

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

Looks good to go once the merge conflicts are fixed.

After merge, confirm if the Action runs successfully.

@kachkaev kachkaev merged commit d2f7a85 into main Feb 22, 2022
@kachkaev kachkaev deleted the ak/remove-duplicate-yarn-lock branch February 22, 2022 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deps Relates to third-party dependencies (area) area/infra Relates to version control, CI, CD or IaC (area)
Development

Successfully merging this pull request may close these issues.

3 participants