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

Fix Node.js 4 support with Proxy #5489

Merged
merged 1 commit into from
Feb 7, 2018
Merged

Fix Node.js 4 support with Proxy #5489

merged 1 commit into from
Feb 7, 2018

Conversation

ai
Copy link
Contributor

@ai ai commented Feb 7, 2018

Summary

Because many projects must support Node.js 4 until LTS end (August 2018) we decided for silent support of Node.js. For example, big projects like Autoprefixer and PostCSS must support Node.js 4, so we run tests on Travis CI with Node.js 4.

Proxy is used for a small win32 fix. Maybe it could be fixed even without Proxy, but I made a small if to be clean.

Test plan

Since it is silent support of Node.js 4 I didn’t add tests.

@ai
Copy link
Contributor Author

ai commented Feb 7, 2018

Right now Browserslist 3.0 release is blocked by this issue. Babel 7 needs Browserslist 3.0 too :). “global village” :D.

@cpojer cpojer requested a review from mjesun February 7, 2018 15:17
delete real[name];
delete lookup[name];
if (typeof Proxy === 'undefined') {
return real;
Copy link
Contributor

@mjesun mjesun Feb 7, 2018

Choose a reason for hiding this comment

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

real is an empty object here, since it's just created based on the prototype. A better fix would be to do, just after the signature of the method:

if (typeof Proxy === 'undefined') {
  return deepCyclicCopy(process.env);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Wait a second. Do you have some deepCyclicCopy method already in the project?

@codecov-io
Copy link

Codecov Report

Merging #5489 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5489   +/-   ##
=======================================
  Coverage   61.66%   61.66%           
=======================================
  Files         213      213           
  Lines        7069     7069           
  Branches        3        4    +1     
=======================================
  Hits         4359     4359           
  Misses       2709     2709           
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-util/src/create_process_object.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9797f23...9191b79. Read the comment docs.

@ai
Copy link
Contributor Author

ai commented Feb 7, 2018

@mjesun done

@mjesun
Copy link
Contributor

mjesun commented Feb 7, 2018

Cool, but you can discard the other commit 😉

@ai
Copy link
Contributor Author

ai commented Feb 7, 2018

@mjesun I merged commits (you can do the same with “Squash me Merge” option on GitHub pull requests).

@mjesun
Copy link
Contributor

mjesun commented Feb 7, 2018

What i meant is that you just need the three lines I added in the review; the rest can be removed 🙂

@ai
Copy link
Contributor Author

ai commented Feb 7, 2018

@mjesun :D cleaned up

@mjesun
Copy link
Contributor

mjesun commented Feb 7, 2018

@thymikee ... and the tar bug again :(

@mjesun mjesun merged commit c8d1c79 into jestjs:master Feb 7, 2018
@ai
Copy link
Contributor Author

ai commented Feb 7, 2018

Thanks :). Waiting for release =^_^=.

@ai
Copy link
Contributor Author

ai commented Feb 8, 2018

@mjesun could you please release it. Browserslist is still broken.

@SimenB
Copy link
Member

SimenB commented Feb 8, 2018

Needs #5496 before release

@ai
Copy link
Contributor Author

ai commented Feb 8, 2018

@SimenB #5496 was merged :) (Autoprefixer release is blocked by this issue too)

@jdalton
Copy link
Contributor

jdalton commented Feb 8, 2018

Thanks @ai!

@SimenB Can this package be bumped sooner than later to fix the Node 4 compat regression?

@SimenB
Copy link
Member

SimenB commented Feb 9, 2018

I don't have publish access on npm, so either @mjesun or @cpojer has to do it (or some other FB employee)

@ai
Copy link
Contributor Author

ai commented Feb 9, 2018

@mjesun @cpojer Browserslist 3.0 and Autoprefixer released is blocked 😣. We need it really soon

@mjesun
Copy link
Contributor

mjesun commented Feb 9, 2018

Let me do a patch with the current status :)

@mjesun
Copy link
Contributor

mjesun commented Feb 9, 2018

22.2.2 is out!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants