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

[Web] Add Platform module for web target #23387

Closed
wants to merge 3 commits into from
Closed

[Web] Add Platform module for web target #23387

wants to merge 3 commits into from

Conversation

EvanBacon
Copy link
Contributor

Summary

Added a Platform file for module for instances where internal modules need Platform.
ex:

const Platform = require('Platform');

Changelog

  • Added Libraries/Utilities/Platform.web.js
    [CATEGORY] [TYPE] - Message

Test Plan

everything should pass.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 11, 2019
@pull-bot
Copy link

pull-bot commented Feb 11, 2019

Messages
📖

📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Generated by 🚫 dangerJS against 11b158c

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Please check out Platform.ios for the proper flow typing of this file. Otherwise, yeah let's do this!

@elicwhite
Copy link
Member

Why does this need to live in core? Do we need to have Platform files for all the platforms people make? Platform.windows.js, Platform.vr.js, Platform.firetv.js? How do those projects currently support these additional platform checks without this file currently existing in the repo?

@EvanBacon
Copy link
Contributor Author

EvanBacon commented Feb 12, 2019

@TheSavior the problem with this particular module is that libraries like Dimensions require it. In RNWeb they have a web module for Platform but react-native's version of Dimensions doesn't have access to it. If any module internally accesses Dimensions then it won't know how to properly handle this import.

Attempting to use something very unrelated like AnimatedEvent will create a tricky error. Currently I have a webpack monkey-patch but it adds significant time to the bundler.

Animated/src/AnimatedEvent -> Renderer/shims/ReactNative -> Renderer/oss/ReactNativeRenderer-dev -> Core/InitializeCore -> Devtools/setupDevtools -> WebSocket/WebSocket -> Utilities/Platform which doesn't exist.

A few modules have this same effect. HMRLoadingView, Systrace, and some networking stuff.

@elicwhite
Copy link
Member

While we should probably merge this just to unblock, I'd really like to have a solution that doesn't require all the platforms to add themselves to the core. Could you think about this and see if you can come up with a solution? :)

@elicwhite
Copy link
Member

In the case of Dimensions, it looks like Android is special cased because the object we get from the device is different on iOS than Android:
https://github.com/facebook/react-native/blob/master/Libraries/Utilities/Dimensions.js#L44-L56

Dimensions comes from native here:
https://github.com/facebook/react-native/blob/master/Libraries/Utilities/Dimensions.js#L125-L140

It seems like the platforms should be unified on the data they provide to JS so that it can be calculated consistently. At which point other platforms have a standard API to report dimensions through and it doesn't need platform checks.

@EvanBacon
Copy link
Contributor Author

EvanBacon commented Feb 12, 2019

@TheSavior AFAIK this is (should be) the only place we define the platform OS value it would seem that adding a new file to Platform is the best solution. Perhaps we could read something from the bundler like process.env on web, but that seems hacky and not scalable.
Depending on how native modules work, maybe a platform could have a native platform module with the same name. That native module could then export the name of it's own OS as a constant. That way it's modular and dependent on the platform. On web we need to think of a way to "install" a native module so it could be accessed the same way as iOS and Android, but this should cover windows.

In the case of Dimensions, it looks like Android is special cased because the object we get from the device is different on iOS than Android:

This paradigm seems fine. I think there should be a way to access Platform from within any module. I would rather fix Platform than change every internal module that uses it.

@matthargett
Copy link
Contributor

For an example of how another out-of-tree platform does this:
https://github.com/Microsoft/react-native-windows/blob/master/Libraries/Utilities/Platform.windows.js

Fully agree with @TheSavior , figuring out a way to do this that doesn't require RN itself having a master list of all platforms is key. At my previous job, we were able to bundle RN Windows app using haul which is webpack underneath.

Of course, I'm open to learning something if there's some counter-indication you have in your specific situation :D

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Agree we should figure out a long term plan but I don't think that's a reason to block landing this PR.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 12, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@EvanBacon merged commit daa79b0 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 12, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 12, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 14, 2019
@hramos hramos added Partner p: Expo Partner: Expo labels Feb 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Expo Partner: Expo Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants