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: handle URL objects as column field values (redux) #7762

Closed

Conversation

dhritzkiv
Copy link
Contributor

@dhritzkiv dhritzkiv commented Jun 17, 2021

Description of change

Fixes #5762
See previous PR (which was reverted) #5771 due to #6142

This time, URL isn't imported from the url module (which isn't present in React Native or browser environments). Instead, it's optionally accessed on the globalThis (if present) object, which is supported in node v.10+ and modern browser environments.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@dhritzkiv
Copy link
Contributor Author

Note: the relevant tests won't be run on CI until #7761 is merged

@imnotjames
Copy link
Contributor

Would something like #5097 fix this & other cases where object are passed?

@dhritzkiv
Copy link
Contributor Author

dhritzkiv commented Jun 17, 2021

Would something like #5097 fix this & other cases where object are passed?

Hmm. Could be. Looking through the code, isPlainObject should return false for an instance of URL, since Object.prototype.toString.call(url) would return '[object URL]'.

I'll pull that branch down locally, add the tests from this PR, and let you know.

@dhritzkiv
Copy link
Contributor Author

I think that branch is so outdated that tests won't run properly. :(

@dhritzkiv dhritzkiv force-pushed the url-as-column-field-values-redux branch from 042abff to 4cd04a9 Compare June 17, 2021 19:59
@dhritzkiv dhritzkiv changed the title Handle URL objects as column field values (redux) Fix: handle URL objects as column field values (redux) Jun 18, 2021
…riable for the platform

This is because node v10 doesn’t support `globalThis` (but do support `URL` globally), which is also likely the case for some older browsers as well.
@dhritzkiv dhritzkiv changed the title Fix: handle URL objects as column field values (redux) fix: handle URL objects as column field values (redux) Jun 21, 2021
@imnotjames
Copy link
Contributor

I believe this should be fixed on the master branch now. I've included your tests.

I'll be closing this PR.

@imnotjames imnotjames closed this Jul 5, 2021
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.

Using URL as a rich column type breaks
2 participants