Skip to content

chore: remove the unused @chemaxon/marvinjs dependency - #115

Open
matthewberry wants to merge 1 commit into
mainfrom
mjberry/remove-chemaxon
Open

matthewberry wants to merge 1 commit into
mainfrom
mjberry/remove-chemaxon

Conversation

@matthewberry

Copy link
Copy Markdown
Collaborator

Why it is not needed

MarvinJsModule was imported in app.module.ts but <app-marvin-js-editor> appears in no template, so the editor never rendered. The only things that consumed the package were the assets/styles/scripts entries in angular.json (which copied the MarvinJS distribution into assets/marvin-js/ and bundled marvinjslauncher.js, util.js, webservices.js into scripts.js) and the MarvinJsEditorComponent that called MarvinJSUtil.getEditor on an iframe pointing at /demo.html. That iframe URL was only proxied in proxy.conf.json for ng serve; in the nginx image /demo.html falls through to index.html, so the editor could not have worked in a deployed container even where it was reachable.

The fleet moved to Ketcher (served through the /ketcher proxy) for the structure editor. This PR removes the leftover MarvinJS/Chemaxon plumbing so the repo no longer needs a Chemaxon license, the NPM_TOKEN secret, or the Chemaxon Artifactory registry.

What changes

  • .github/workflows/docker.yml

  • .npmrc (deleted)

  • README.md

  • angular.json

  • docker-compose.marvin.yaml (deleted)

  • package-lock.json

  • package.json

  • proxy.conf.json

  • src/app/app.module.ts

  • src/app/components/chemscraper/marvinjs/index.ts (deleted)

  • src/app/components/chemscraper/marvinjs/marvinjs-editor.component.ts (deleted)

  • src/app/components/chemscraper/marvinjs/marvinjs.d.ts (deleted)

  • src/app/components/chemscraper/marvinjs/marvinjs.module.ts (deleted)

  • package.json: @chemaxon/marvinjs removed.

  • package-lock.json: its entry removed, and 2 resolved URLs that pointed at hub.chemaxon.com:443/artifactory/api/npm/npm/ (the Artifactory proxy of npmjs, which the old .npmrc had made the default registry) repointed at registry.npmjs.org. Tarballs and integrity hashes are unchanged; Artifactory serves npmjs tarballs byte-for-byte, and npm ci verifies every hash.

  • angular.json: the 12 MarvinJS assets/styles/scripts entries (6 per configuration).

  • .npmrc: Chemaxon registry lines removed (file deleted; nothing else was in it).

  • Workflows: the hub.chemaxon.com auth line and its NPM_TOKEN comment. Where that line was the only thing creating $HOME/.npmrc, it is replaced by touch $HOME/.npmrc so the Docker build secret mount still has a file.

  • docker-compose.marvin.yaml, the MarvinJS backend entries in proxy.conf.json, the README's Chemaxon registry setup block.

  • The MarvinJS editor component (marvinjs/ directory: editor component, module, type declarations) and its MarvinJsModule registration in app.module.ts, since it existed only to host the package.

Verification

npm ci --userconfig /dev/null (no registry credentials at all) followed by ng build succeeds locally on this branch (Angular 15, webpack builder): every repointed tarball downloaded from npmjs with its original integrity hash, and the production bundle builds without the MarvinJS scripts. grep -rl chemaxon over the repo (excluding node_modules, dist, .git) is empty.

After merge

The NPM_TOKEN repository secret is no longer referenced by any workflow and can be deleted.

The MarvinJS editor never rendered in this app (the structure input uses
Ketcher, or the editor was registered but placed in no template), so the
Chemaxon package, its angular.json asset/script/style entries, the
Artifactory registry in .npmrc, the NPM_TOKEN workflow auth, the dev
proxy entries, the compose file and the editor component are removed.
Lockfile tarball URLs that went through the Chemaxon Artifactory proxy
are repointed at registry.npmjs.org with their integrity hashes intact.
@matthewberry

Copy link
Copy Markdown
Collaborator Author

Checked: on the branch, none of the workflow files, .npmrc, README.md, angular.json, package.json, proxy.conf.json, Dockerfile, nginx/default.conf, or the remaining marvinjs-input component files reference chemaxon, NPM_TOKEN, MarvinJSUtil or assets/marvin-js (each file fetched and grepped). The kept <app-marvinjs-input> wrapper renders <app-ketcher>. The docker check, which builds the production image from the rewritten lockfile, is green on this PR.

Findings:

  1. (low) Unrelated whitespace change. The package.json hunk also deletes four blank lines inside scripts. Harmless, but it is not part of removing the dependency and widens the diff for no reason.
  2. (low) Body inaccuracy. The verification text says "every repointed tarball downloaded from npmjs with its original integrity hash", but this repo's lockfile was never resolved through Artifactory: the package-lock.json diff is 11 deleted lines (the @chemaxon/marvinjs entries) and no resolved URL changes. The sentence is shared with somn/novostoic, where the repointing did happen.
  3. (low, pre-existing, but this is the PR that makes it wrong) MarvinJS branding on a Ketcher button. results.component.html line 87 still renders <img src="assets/MarvinJS_Logo.svg" alt="MarvinJS Logo"> on the "Draw / Upload" button, and that button opens the <app-ketcher> dialog. With the package gone the label is now purely misleading; worth fixing here or in a follow-up along with src/assets/MarvinJS_Logo.svg.

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.

1 participant