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

changed port array to store based on id #725

Closed
wants to merge 6 commits into from
Closed

changed port array to store based on id #725

wants to merge 6 commits into from

Conversation

brizzbuzz
Copy link

@brizzbuzz brizzbuzz commented Sep 18, 2020

Checklist

  • The code has been run through pretty yarn run pretty
  • The tests pass on CircleCI
  • You have referenced the issue(s) or other PR(s) this fixes/relates-to
  • The PR Template has been filled out (see below)
  • Had a beer/coffee because you are awesome

What?

In reference to #724 I think it is unwise to store ports based on a user provided name, as this can easily create clashes. The unique ID is a less opinionated choice

Why?

Well, personally I think I lost a couple hairs on my head because of this, hopefully this will prevent future hair loss :D

How?

Changed addPort and a couple other methods to store/pull ports based on ID instead of name

Feel good image:

LOL

@dylanvorster
Copy link
Member

At a surface level this looks like a small change, but this would needs further testing preferably with the community to make sure things still work. Projects that already extend from this might make use of the internal behaviour that is now changing and thus might require a rework.

@brizzbuzz
Copy link
Author

Understandable, happy to leave it open and see where it goes... The workaround is rather simple, I just added another field that is my "name name" and then just make the "port name" unique :D Obviously not ideal but it'll do

@adaladam
Copy link
Contributor

adaladam commented Oct 3, 2020

this one is related to pull request #663

@brizzbuzz brizzbuzz closed this Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants