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

chore(gatsby): migrate webpack-hmr-hooks-patch to TypeScript #22280

Merged
merged 6 commits into from
Mar 19, 2020

Conversation

sasurau4
Copy link
Contributor

Description

Part of #21995

I think the patch assume to run on browsers and use eslint-disable and @ts-ignore. If you have any other ideas, I hope you would pointed it out.

Documentation

Related Issues

Part of #21995

@sasurau4
Copy link
Contributor Author

sasurau4 commented Mar 15, 2020

@blainekasten I investigated the reason why CI failed. It's because the transpiled webpack-hmr-hooks-patch.js are bit different and the patch is invalidated.

webpack-hmr-hooks-patch.js is transpiled to following.

"use strict";

const originalFetch = global.fetch;
delete global.fetch;
module.exports = require(`react-hot-loader/webpack`);
global.fetch = originalFetch;
//# sourceMappingURL=webpack-hmr-hooks-patch.js.map

On the other hand, webpack-hmr-hooks-patch.ts is transpiled to following.

"use strict";

var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefault");

exports.__esModule = true;
exports.default = void 0;

var _webpack = _interopRequireDefault(require("react-hot-loader/webpack"));

exports.default = _webpack.default;

/* eslint-disable @typescript-eslint/ban-ts-ignore */
// @ts-ignore: This file assume run at browsers.
const originalFetch = global.fetch; // @ts-ignore

delete global.fetch;
global.fetch = originalFetch;
//# sourceMappingURL=webpack-hmr-hooks-patch.js.map

So, the order of deleting and restoring global.fetch is broken.
I think it would be better to move webpack-hmr-hooks-patch.js to cache-dir or not migrating this file to typescript.
How about it?

@blainekasten
Copy link
Contributor

@sasurau4 thank you so much for your exploration here. I think you are right that this file ultimately should remain as a JS file. So here is what I think we should do.

Let's add a comment to the top of the file that says this file should remain as a JS file, with a reason why and link to this PR. I'd be happy to merge that so we know in the future why this one file stayed a JS file.

@blainekasten blainekasten added the status: awaiting author response Additional information has been requested from the author label Mar 19, 2020
@sasurau4 sasurau4 requested a review from blainekasten March 19, 2020 11:36
@sasurau4
Copy link
Contributor Author

@blainekasten OK, Thanks 👍

@blainekasten blainekasten added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: awaiting author response Additional information has been requested from the author labels Mar 19, 2020
Copy link
Contributor

@blainekasten blainekasten left a comment

Choose a reason for hiding this comment

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

This looks great to me, let's merge it!

Thank you so much for contributing to our TypeScript refactor! We have more work to do and we would love to have you stay involved in our transition. Please submit more PRs! 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants