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

Removed code duplication between 2 files. #2805 #3384

Merged

Conversation

Harshit-7373
Copy link
Contributor

@Harshit-7373 Harshit-7373 commented Mar 10, 2025

Fixes #2805

Changes:

  1. A common function was used in both files. To avoid duplication, I created a shared folder and added a new file (resolveUtils.js), where I wrote the common function along with the necessary import statements.
    Imported the shared function (resolveUtils.js) into both files and removed the redundant function definitions.
  2. Updated and corrected the RegExp.test() code to improve efficiency.
  3. Added Explanatory comments in the files wherever necessary.
  4. Added Unit tests.

Answers to the Questions Asked in the PR Description:

  1. There was a function named (resolvePathsForElementsWithAttribute) which was common in both the files. The exact code which was same in both the files was : - export function resolvePathsForElementsWithAttribute(attr, sketchDoc, files) { const elements = sketchDoc.querySelectorAll([${attr}]); const elementsArray = Array.prototype.slice.call(elements); elementsArray.forEach((element) => { if (element.getAttribute(attr).match(MEDIA_FILE_REGEX)) { const resolvedFile = resolvePathToFile(element.getAttribute(attr), files); if (resolvedFile && resolvedFile.url) { element.setAttribute(attr, resolvedFile.url); } } }); }

  2. Some parts of the code were similar but had slight differences due to the client vs. server environment requirements.

  3. Since the /embed/ endpoints are currently disabled due to phishing concerns, the server-side version of this code may no longer be necessary. If its primary purpose was handling these endpoints and they are no longer in use, it would make sense to remove it unless there is a plan to re-enable them securely.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@Harshit-7373
Copy link
Contributor Author

An image showing that the unit test has passed :
Screenshot 2025-03-11 at 1 11 12 AM

@Harshit-7373
Copy link
Contributor Author

I had made a PR on the same issue earlier, but due to some errors, I had to close that one. This is a fully updated pull request with the necessary unit tests, as we discussed earlier. @raclim , Please review it if possible.

Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! I think this overall feels good to me! I changed a bit of the names and removed a few extra comments that felt self-explanatory. I also added in a bit of documentation for the utility function, which is a practice we generally want to hold!

@raclim raclim merged commit 981da2e into processing:develop Mar 13, 2025
1 check passed
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.

Repeated code between server previewGeneration.js and client EmbedFrame.jsx
2 participants