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

fix: Remove slugify dependency #786

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

alexs-mparticle
Copy link
Collaborator

@alexs-mparticle alexs-mparticle commented Oct 9, 2023

Instructions

  1. PR target branch should be against development
  2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

  • Replace Dependency on Slugify without using any polyfills or JS functions that are incompatible with ES5.

Testing Plan

  • Was this tested locally? If not, explain why.
  • Using a sample app, attempt to create a config with a data plan that does not conform to our Data Plan naming format.
  • Verify that this will be rejected via an error in the Dev Console.
  • Verify that a data plan that is correct will send valid data to the live stream.

Invalid Data Plan Slug

Screenshot 2023-10-09 at 4 20 57 PM

Valid Data Plan Slug

Screenshot 2023-10-09 at 4 22 59 PM

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

@alexs-mparticle alexs-mparticle changed the base branch from master to development October 9, 2023 20:12
@alexs-mparticle alexs-mparticle marked this pull request as ready for review October 9, 2023 20:23
@alexs-mparticle
Copy link
Collaborator Author

Closing and reopening to re-run checks

src/utils.ts Outdated
// TODO: Refactor this to a regex
const isDataPlanSlug = (str: string): boolean =>
isString(str) && str === slugify(str);
const toSlug = (value: any): string =>
Copy link
Member

@rmi22186 rmi22186 Oct 11, 2023

Choose a reason for hiding this comment

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

Technically these are not slugs, because slugs can have hyphens. I think we should really call this something more specific to our purposes. I see the backend uses the term PlanSlug, which because it's in a file named DataPlan might make sense, so I think the following is a goodr renaming:

Suggested change
const toSlug = (value: any): string =>
const toDataPlanSlug = (value: any): string =>

src/utils.ts Outdated
.replace(/[^0-9a-zA-Z]+/g, '_')
: '';

const isDataPlanSlug = (str: string): boolean => str === toSlug(str);
Copy link
Member

@rmi22186 rmi22186 Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
const isDataPlanSlug = (str: string): boolean => str === toSlug(str);
const isDataPlanSlug = (str: string): boolean => str === toDataPlanSlug(str);

Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

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

This looks good overall. I think just renaming it would make most sense since a slug can be hyphenated

Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

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

I'd recommend also removing slugify from package.json, doing an npm install, and also commiting the package-lock.json

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alexs-mparticle alexs-mparticle merged commit 8346bdc into development Oct 17, 2023
26 of 34 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 17, 2023
## [2.23.6](v2.23.5...v2.23.6) (2023-10-17)

### Bug Fixes

* Remove slugify dependency ([#786](#786)) ([8346bdc](8346bdc))
@mparticle-automation
Copy link
Collaborator

🎉 This PR is included in version 2.23.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants