Skip to content

[NO-TICKET-0001] Fixes a reference to a null variable#4

Closed
joelrfcosta wants to merge 1 commit intodppfrom
bug/no-ticket-0001
Closed

[NO-TICKET-0001] Fixes a reference to a null variable#4
joelrfcosta wants to merge 1 commit intodppfrom
bug/no-ticket-0001

Conversation

@joelrfcosta
Copy link

What did you implement:

Implemented a fix to an issue where it was possible to call a property from a null variable

How did you implement it:

Added a null check to the condition.

@joelrfcosta joelrfcosta self-assigned this Aug 3, 2017
@joelrfcosta joelrfcosta added the bug label Aug 3, 2017
@ericqweinstein
Copy link

Does this amount to a private fork of this library? If so, I'd much rather open the PR against the main project and just update our dependencies once they merge the fix.

@joelrfcosta
Copy link
Author

joelrfcosta commented Aug 3, 2017

I've thought following that approach too, but the main project have 131 PRs Open so it could take way too long for us to be merged (without this the https://github.com/foxbroadcasting/fng-cms/pull/248 will not work).

@bernardodiasc
Copy link

@ericmelkerson I would agree with you, but the "once they merge the fix" can take more time than we have available sometimes (if they happen to merged). That was the reason we moved to this approach.

@ericqweinstein
Copy link

@joelrfcosta @bernardodiasc Understood. Could we still make the upstream PR and note that we should swap out our private fork for the original project when they eventually merge a fix? This makes me nervous because I've seen this bite developers too many times (small changes accumulate to the point where the project is very different from the original, then some valuable new feature is merged upstream and the team can't take advantage of it).

@bernardodiasc
Copy link

That's good thing to do. In DPP, the idea is to use this 3th party component always with the wrapper https://github.com/foxbroadcasting/fng-cms/blob/development/src/shared/components/inputs/SelectInput/, this way we can track the specific use cases we support on DPP in a single place (example https://github.com/foxbroadcasting/fng-cms/blob/development/src/shared/components/inputs/StringList/StringList.js#L4).

I think it could be good to fill the docs with specs https://github.com/foxbroadcasting/fng-cms/wiki/select, because from time to time it can be possible to simply drop all the temporary solution in favor of upstream if that simply match specs, but well, we don't have documented/tested specs yet for many things :/

the follow up for documentation is https://fng-jira.fox.com/browse/DPP-132, but it's quite generic, should be better to split in more specific documentation tasks. :)

@maxim-smotrov
Copy link

maxim-smotrov commented Aug 9, 2017

@joelrfcosta @bernardodiasc @ericqweinstein They already fixed this in there repository: JedWatson#1508.
And so happened that I also need that fix. I found it in there code and created a PR with the same changes:
#5

That'd be better to follow their code so we won't have issues when we switch back to the base.

CC @jangelon

@jangelon
Copy link

jangelon commented Aug 10, 2017

@joelrfcosta I merged Max's PR which replicated the upstream fix, and updated the npm package. Can you try the new 1.0.4 package and see if your issue is resolved as well now, or if we still need another PR to fix it?

@jangelon
Copy link

Closing old PR, we'd merged an alternate PR that also included the fix.

@jangelon jangelon closed this Feb 20, 2018
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.

5 participants