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

Add support for Babel 7 + RN 0.57

Merged
merged 30 commits into from Oct 10, 2018
Merged

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Sep 17, 2018

I've been testing this with React Native 0.57, Babel 7 stable, and Android. I'm not sure if it works with RN 0.56 and the corresponding Babel 7 beta, or on iOS.

Changes

  • Replace babel-preset-react-native with metro-react-native-babel-preset
  • Upgrade all babel dependencies to equivalent v7 packages
  • Update peerDependency on react-native to ^0.57
  • Slight refactor of babelPlugin.js to account for the changed API of @babel/template
  • Changed the haste resolver plugin to account for the removal of @providesModule annotations in globally importable libs. Instead it includes all libraries in the Libraries directory and assumes that their file names are the name that should be imported. Not sure what is wrong with normal relative imports...
  • Update all fixtures to use react-native 0.57.1
  • Upgrade react-hot-loader to v4, and remove the additional monkey patching of React.createElement and co. HMR seems to work fine without it and the monkey patching no longer works on v4. For me it did not previously work with the extra monkey patching + react-native-maps, now it does, even though there is a scary error.
  • Automatically add react-hot-loader/patch to entry points instead of patching it into the user's entry file with the babel plugin.

@hedgepigdaniel hedgepigdaniel changed the title WIP: Add support for Babel 7 + RN 0.57 Add support for Babel 7 + RN 0.57 Sep 24, 2018
@thymikee thymikee requested a review from zamotany Sep 24, 2018
@thymikee thymikee requested a review from jukben Sep 24, 2018
@greg-dove
Copy link
Contributor

greg-dove commented Oct 1, 2018

fyi, I've been trying this along with using RN0.57.0 against our code which is a lerna based monorepo. So far, it has been a mostly good experience - thanks!
I have however seen an issue that is new vs. the code immediately prior to these changes. It seems related to react-hot-loader. I was able to restore the previous behavior by making the following change in haul/hot/patch.js. I could be doing something 'quite wrong' here... I am just trying to address the issue (which was related to a comparison of scene.type in react-native-router-flux for Drawer).

var patch = require('react-hot-loader/patch');
if (__DEV__) patch.default.disableProxyCreation = true;
module.exports = patch;

as opposed to your original:
require('react-hot-loader/patch');

I'm not suggesting this is a necessary change, because I am not at all familiar with this code - I'm just pointing out what I needed to do in order to restore my dev behavior for the codebase I am currently working on. If it's not correct, then it might be a helpful clue. Please let me know if I can provide any more information.

(also, in terms of my testing: So far I have only been working with android on windows, I will try iOS tomorrow.)
Update: I have the same experience on iOS.
Following on from discussions in discord I can also swap the haul/hot/patch.js entry in the haul.config webpack config entry array for a local project file which has the same code in it, and that serves as a local monkeyish 'patch' for now (not a solution, but a current workaround):

    config.entry = config.entry.map(
        entryItem => (entryItem.indexOf(['haul' , 'hot' , 'patch.js'].join(path.sep)) !== -1) ? path.join(__dirname, 'haulHotPatch.js') : entryItem
    );

@chaseholland
Copy link
Contributor

chaseholland commented Oct 2, 2018

Expanding on what @greg-dove said above -- other than react-hot-patch, this is looking really good on iOS and Android for us. I was able to work around the hot patch issues by removing the entry file and plugin. Here are the relevant parts of my haul config in case it helps anyone else:

        // Remove react-hot-patch as it causes issues with bundling
        config.entry = config.entry.filter((entry) => {
            return !entry.includes('hot/patch.js');
        });
        // Remove the react-hot-loader config since we don't use it and it causes runtime issues
        config.module.rules.forEach((rule) => {
            if (Array.isArray(rule.use)) {
                rule.use.forEach((use) => {
                    if (use.options && use.options.plugins && Array.isArray(use.options.plugins)) {
                        use.options.plugins = use.options.plugins.filter((plugin) => { return !plugin.includes('react-hot-loader') });
                    }
                });
            }
        });

Copy link
Member

@zamotany zamotany left a comment

I'm going to approve this, since it's looking really good, but before we merge the following needs to be addressed:

That being said, thank you @hedgepigdaniel for your contribution, we really appreciate your effort and we're looking forward to merge this PR 😉 👍 💪

hot/patch.js Outdated
if (module.hot && process.env.NODE_ENV !== 'production') {
require('../src/hot/client/hotPatch');
}
require('react-hot-loader/patch');
Copy link
Member

@zamotany zamotany Oct 9, 2018

Choose a reason for hiding this comment

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

As @greg-dove said, we need to disable proxy creation, otherwise HMR won't persist the state:

require('react-hot-loader/patch').default.disableProxyCreation = true;

@thymikee thymikee merged commit ae464ea into callstack:next Oct 10, 2018
1 check passed
@jukben
Copy link
Contributor

jukben commented Oct 10, 2018

Great job everyone involved! ❤️

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.

None yet

7 participants