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

APEX-689: Migrated Algolia libraries to the cartridge #14

Merged
merged 9 commits into from
Mar 16, 2023

Conversation

ede-somogyi-algolia
Copy link
Contributor

@ede-somogyi-algolia ede-somogyi-algolia commented Mar 13, 2023

This PR contains changes from APEX-497 and is pointed to the newly-created develop branch.
Closing down the other PR for clarity and adding the changes here:

Changes from APEX-497:

  • refactored package.json, removed unnecessary packages;
  • updated Node.js packages, fixed security vulnerabilities;
  • fixed JS linting (handled via sgmf-scripts);
  • updated .eslintignore: removed/rewrote some ignored paths;
  • updated .eslintrc.json;
  • added .prettierrc config;
  • fixed repository URL in package.json;
  • minimum Node version is now at least v14 instead of the previous hard 14.

Changes from APEX-689:

  • as per SFCC recommendations, external Algolia libraries were moved to package.json as devDependencies and also committed to the repository instead of loading them from a CDN
  • other external (non-Algolia) libraries were also moved to devDependencies and committed to the repo (Hogan.js, Preact);
  • the changes are transparent to the end user, no additional steps are necessary when installing the cartridge;
  • version numbers were kept the same as before and pinned down (exact version numbers instead of ^);
  • removed Polyfill as it is no longer needed;
  • updated CircleCI config (Node version and linter script to run);
  • moved the scripts around in the directory tree a bit so that all external libraries can be .eslintignore'd together;
  • added JSDoc comments to the scripts that needed them.

Outstanding issues:

  • CircleCI still fails due to the unit tests throwing errors -- will be in the scope of another ticket;
  • autocomplete-js@1 was left as-is because it requires algoliasearch@4 as a dependency, but the cartridge currently uses v3;
  • updating algoliasearch to v4 is pending as it is a major version change and as such may contain breaking changes -- will be in the scope of another ticket.

Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

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

This branch is based off of feature/APEX-497

Then you can make it point on that branch instead of master, so the diff doesn't contain changes from the other PR

Otherwise it looks good, only small comments.
(I'm just not convinced by the need to have an extra dependencies to copy a few files)

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@sbellone
Copy link
Collaborator

Oh also, there is an error on the CI when installing packages

@ede-somogyi-algolia ede-somogyi-algolia changed the base branch from master to develop March 14, 2023 14:20
@ede-somogyi-algolia
Copy link
Contributor Author

  • as discussed, I created a develop from master and pointed the pull requests to that.
  • fixed the build error from CircleCI -- the error from the unit tests will be fixed in a different PR
  • moved the Algolia scripts to devDependencies instead of dependencies as the minified versions are committed to the repo
  • switched to using exact version numbers for the Algolia devDependencies
  • removed Polyfill as discussed
  • committed the previously remotely-included Preact to the repo as well

Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

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

Great! Almost there. There are some linting errors in the build.

attribute: "name",
tagName: "em"
})}</a>
hit: item,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This indentation is a bit weird no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed indentation.

package.json Outdated
"eslint-plugin-jquery": "^1.5.1",
"eslint-plugin-prettier": "^4.2",
"hogan.js": "1.0.2",
"htm": "^3.1.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"htm": "^3.1.1",
"htm": "3.1.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.

Pinned down version number.

@ede-somogyi-algolia ede-somogyi-algolia merged commit de396c9 into develop Mar 16, 2023
@sbellone sbellone deleted the feature/APEX-689 branch March 16, 2023 15:21
@ede-somogyi-algolia ede-somogyi-algolia self-assigned this Apr 3, 2023
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.

2 participants