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

const injected in runtime code #297

Closed
Donov4n opened this issue Jan 20, 2021 · 7 comments · Fixed by #337
Closed

const injected in runtime code #297

Donov4n opened this issue Jan 20, 2021 · 7 comments · Fixed by #337

Comments

@Donov4n
Copy link

Donov4n commented Jan 20, 2021

Thanks for this super cool project!

In RefreshModule.runtime.js, const are injected in runtime code and will not be transpiled (since it's a loader injected code).

This is critical in old browser that do not support const in strict mode for example.

Elsewhere in the codebase i've seen a supportsConst method that seems to handle this specific case, maybe the code injected by RefreshModule.runtime.jsshould do the same.

@pmmmwh
Copy link
Owner

pmmmwh commented Jan 21, 2021

Hi, that is actually done deliberately on purpose. All modern browser since 2016 have working implementations of const and let (and also IE11 supports it). Are you facing specific issues where that would break your code?

@Donov4n
Copy link
Author

Donov4n commented Jan 21, 2021

We are trying to replace RHL with react-refresh but we need to support old browsers (like Chrome 30-40 f.e) but these const are breaking our code with errors like "SyntaxError: Use of const in strict mode".

It seems sad to me to not support all the projects that need to take into account old browsers (not only IE) just because of these few line 😞 but if it's done deliberately, maybe this should be explicitly said in the README / FAQ (?).

@pmmmwh
Copy link
Owner

pmmmwh commented Jan 21, 2021

We are trying to replace RHL with react-refresh but we need to support old browsers (like Chrome 30-40 f.e) but these const are breaking our code with errors like "SyntaxError: Use of const in strict mode".

It seems sad to me to not support all the projects that need to take into account old browsers (not only IE) just because of these few line 😞 but if it's done deliberately, maybe this should be explicitly said in the README / FAQ (?).

I understand the frustration - I'll look into it, in particular I don't think we rely on any scoping behaviours from const and let.

@TrySound
Copy link

TrySound commented Jan 21, 2021

Just wondering why do you support chrome 30-40? They did not get security updates for 5 or more years already.
Is it even possible to test them?

@Donov4n
Copy link
Author

Donov4n commented Jan 21, 2021

Just wondering why do you support chrome 30-40? They did not get security updates for 5 or more years already.
Is it even possible to test them?

This bug can be seen in all browsers versions that don’t support const / let in strict mode, not only Chrome 30-40.
(f.e. If you want to support iPhone 4S, iPad 2 ...)

For our case, we support Chrome since v30 because the platforms (old tv) where our project is run can’t update to newer version of chrome.

@pmmmwh
Copy link
Owner

pmmmwh commented Jan 21, 2021

To be honest, the pragmatist in me definitely does not want to support these browsers, but given React supports browsers down to IE9 I'm not sure about how compelling our argument on this case would be.

However, given that Webpack 5 now does have APIs to selectively target specific environments and we already lean on it with the crucial runtime init code, maybe it makes sense to actually use that more to ensure not only compat with older browsers but also pure ESM environments (#294).

It does make the template code more difficult to maintain and lint because now it is not really pure JavaScript.

@Donov4n
Copy link
Author

Donov4n commented Jan 21, 2021

Like said, it’s not only for Chrome 30-40, this bug is viewable on iOS 9, which imply supporting (or not) iPad 2, iPhone 4S ...

+1 for the fact that React itself supports IE >= 9.

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 a pull request may close this issue.

3 participants