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.

Sign up for GitHub

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

Jump to bottom

Fix regression in dynamic resolution for out-of-tree platforms #20662

Closed
wants to merge 15 commits into from
Closed

Fix regression in dynamic resolution for out-of-tree platforms #20662

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d314ef8
Initial changes to make platform resolution dynamic for out-of-tree p…
matthargett Aug 14, 2018
94425cc
Merge branch 'master' of github.com:matthargett/react-native into dyn…
matthargett Aug 15, 2018
a5190b7
Revert fixes for lint/prettier failures.
matthargett Aug 16, 2018
a51bffe
Merge branch 'master' of github.com:facebook/react-native into dynami…
matthargett Aug 16, 2018
771df00
Take Name.es6.js and Name.es7.js into account for regexp.
matthargett Aug 16, 2018
403930c
Revert prettier auto-fix.
matthargett Aug 16, 2018
1f7f362
Adjust comment and regexp. Note weird error when only two platforms a…
matthargett Aug 17, 2018
8dc4fe7
From @rozele: React Native overrides the settings for, e.g., , which…
matthargett Aug 22, 2018
9edfa98
Merge branch 'master' of github.com:facebook/react-native into dynami…
matthargett Aug 22, 2018
5207cd1
Remove hard-coded platform paths.
matthargett Aug 22, 2018
0e72049
hasteImpl will be overridden via metro configurations of out-of-tree …
matthargett Aug 23, 2018
3159e0c
Merge branch 'master' of github.com:facebook/react-native into dynami…
matthargett Aug 23, 2018
d366ea4
Update tests to remove out-of-tree platform specifics
matthargett Aug 23, 2018
9ce4e7a
Fix comment
matthargett Aug 23, 2018
5733fe1
My synthetic tests didn't capture real-world pathing. This should fix…
matthargett Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jest/hasteImpl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const NAME_REDUCERS /*: Array<[RegExp, string]> */ = [
// strip .js/.js.flow suffix
[/^(.*)\.js(\.flow)?$/, '$1'],
// strip .android/.ios/.native/.web suffix
[/^(.*)\.(android|ios|native|web|windows|dom)$/, '$1'],
[/^[^\.]+(\.es\d)?$/, '$1'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this could use a comment, not sure how it works

Copy link
Contributor

@rozele rozele Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I ran a test on the regex, and I can't get it to match Foo.native.js. This is the code that I had commented in Slack about. Unfortunately, hasteImpl is dynamically required by jest-haste-map, so it's not trivial to inject "plugin" platform extensions.

However, we can move to a model where plugin platforms must create their own metro.config.js and hasteImpl.js file with the platform extensions in the regex for getHasteName, but that would mean it would be more challenging to add plugin platforms to projects that already have their own metro.config.js. We could probably use babel to modify existing metro.config.js files and add hasteImplModulePath if it's not already there, and just throw a warning if it's already there pointing to instructions on how to add the necessary platform behavior to their hasteImpl.js.

Copy link
Contributor

@hramos hramos Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like this regex aims to strip out .es6 suffixes, but it needs one more capture group. As it stands right now, it would take index.es6 and return .es6, which is unlike the other reducers that strip out .js or .flow.js.

It looks like you want to use ^([^\.]+)(\.es\d)?$ here, which would return index in the above example.

This doesn't match the comment from the line above it, however.

];

const haste = {
Expand Down
2 changes: 1 addition & 1 deletion local-cli/core/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ const defaultConfig = {
hasteImplModulePath: require.resolve('../../jest/hasteImpl'),

getPlatforms(): Array<string> {
return ['ios', 'android', 'windows', 'web', 'dom'];
return ['ios', 'android', ...Object.keys(pluginPlatforms)];
},

getProvidesModuleNodeModules(): Array<string> {
Expand Down