esm: fixup process Proxy to handle toString correctly #25963

guybedford · 2019-02-06T15:43:48Z

This fixes the following --experimental-modules bug:

import process from 'process';
// returns [object Object] instead of [object process]
console.log(Object.prototype.toString.call(process));

//cc @devsnek

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-02-06T15:43:50Z

@guybedford build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2527/pipeline

bmeck · 2019-02-06T15:50:33Z

Should we just set this unconditionally when we setup the process Object to begin with? Why do it lazily?

lib/internal/bootstrap/loaders.js

joyeecheung · 2019-02-07T01:42:54Z

Should we just set this unconditionally when we setup the process Object to begin with? Why do it lazily?

The process object is not a proxy, only the one exported by import process from 'process' is.

devsnek · 2019-02-07T01:52:26Z

This seems kind of backward, why not just install Symbol.toStringTag on the process object anyway?

zenparsing · 2019-02-07T22:14:28Z

I've run into a few test failures when experimenting with removing the experimental-modules flag. I'm worried that proxifying native modules is going to cause other breaking changes in addition to the one described here.

jdalton

I'm 👎 on this work. Next week (2019-02-13) the Module WG will resolve a bunch of things that affect the direction and features of experimental modules. I don't see a strong pull for this addition when it will likely change come April.

guybedford · 2019-02-17T12:19:23Z

This is a bug fix to existing behaviour. If you're against the existing behaviour, sure, but I don't think complaints against the existing behaviour should stop bug fixes from landing against it, that seems very counter-productive.

guybedford · 2019-02-17T13:01:14Z

@jdalton the freeze on modules in Node.js core was for new features not bug fixes as well. Note also that the current minimal implementation also retains this work as well so that this isn't currently slated for any changes or removals either.

guybedford · 2019-02-17T13:05:35Z

This seems kind of backward, why not just install Symbol.toStringTag on the process object anyway?

@devsnek could you share a code example of what you mean here? I thought that was what this is doing already?

devsnek · 2019-02-17T13:51:53Z

@guybedford right now the toString behaviour comes from setting a native name in the V8 API. instead of doing that, let's just put a toStringTag on the original process object, and then no changes to the proxy need to be made.

guybedford · 2019-02-17T17:01:57Z

Ok, I've updated this approach to simply define Symbol.toStringTag on process to begin with.

lib/internal/bootstrap/node.js

danbev · 2019-02-18T07:01:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/20861/

PR-URL: nodejs#25963 Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

guybedford · 2019-02-22T09:05:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/20961/

jasnell · 2019-02-22T16:30:32Z

Manual LGTM as the Review button appears to currently be broken in the Github UI

guybedford · 2019-03-03T15:32:45Z

Running CI again due to a bsd flaky case, just to be sure - https://ci.nodejs.org/job/node-test-pull-request/21165/.

guybedford · 2019-03-03T16:26:57Z

Landed in ccaebde.

PR-URL: #25963 Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

SimenB · 2019-03-07T09:44:06Z

This change has broken Jest (4.5M weekly downloads): jestjs/jest#8069

While this is fixed in Jest on master old versions are still broken. Not sure if just making it writable fixes it?

guybedford · 2019-03-07T10:07:14Z

Sure we can make this writeable, PR welcome.

…

On Thu, 07 Mar 2019 at 11:44, Simen Bekkhus ***@***.***> wrote: This change has broken Jest (4.5M weekly downloads): jestjs/jest#8069 <jestjs/jest#8069> While this is fixed in Jest on master old versions are still broken. Not sure if just making it writable fixes it? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#25963 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkiyhEeqDd4xRRHf5RsbZn0NXbm3bviks5vUN9ygaJpZM4algtR> .

SimenB · 2019-03-07T10:09:45Z

#26488

targos · 2019-03-07T11:54:25Z

Is there anything blocking us from having Jest tested in CITGM?

Edit: Yes. I opened nodejs/citgm#684

targos · 2019-03-07T12:11:28Z

I labeled this dont-land on LTS

SimenB · 2019-03-07T12:12:09Z

We've tried before, but colors were wrong (a whole bunch of Jest's tests fail if terminal colors do not work), ref nodejs/citgm#560 (comment)

Should fix the jest failure on master. * nodejs/node#25963 (comment) * jestjs/jest#8069

* nodejs/node#25963 (comment) * jestjs/jest#8069

guybedford force-pushed the esm-process branch from 7215790 to 4b8cc5f Compare February 6, 2019 15:49

joyeecheung reviewed Feb 7, 2019

View reviewed changes

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved

jdalton suggested changes Feb 8, 2019

View reviewed changes

guybedford force-pushed the esm-process branch from 4b8cc5f to 6453a66 Compare February 17, 2019 17:01

jdalton approved these changes Feb 17, 2019

View reviewed changes

jdalton reviewed Feb 17, 2019

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

addaleax approved these changes Feb 17, 2019

View reviewed changes

devsnek approved these changes Feb 18, 2019

View reviewed changes

TimothyGu approved these changes Feb 19, 2019

View reviewed changes

BridgeAR approved these changes Feb 20, 2019

View reviewed changes

guybedford force-pushed the esm-process branch from 6453a66 to ed15c58 Compare February 22, 2019 09:04

joyeecheung approved these changes Feb 22, 2019

View reviewed changes

guybedford closed this Mar 3, 2019

BridgeAR mentioned this pull request Mar 4, 2019

v11.11.0 proposal #26322

Merged

BridgeAR mentioned this pull request Mar 7, 2019

process: make Symbol.toStringTag writable #26488

Closed

4 tasks

targos mentioned this pull request Mar 7, 2019

Add Jest to lookup nodejs/citgm#684

Closed

targos added dont-land-on-v6.x labels Mar 7, 2019

nikolas mentioned this pull request Mar 7, 2019

Pin node 11 to 11.10.1 ccnmtl/astro-simulations#293

Closed

nikolas added a commit to ccnmtl/astro-simulations that referenced this pull request Mar 7, 2019

:arrow_up" jest 24.3.0

21a17c3

Should fix the jest failure on master. * nodejs/node#25963 (comment) * jestjs/jest#8069

nikolas mentioned this pull request Mar 7, 2019

⬆️ jest 24.3.0 ccnmtl/astro-simulations#294

Merged

nikolas added a commit to ccnmtl/astro-simulations that referenced this pull request Mar 7, 2019

⬆️ jest 24.3.0

ce37166

Should fix the jest failure on master. * nodejs/node#25963 (comment) * jestjs/jest#8069

nikolas added a commit to ccnmtl/astro-simulations that referenced this pull request Mar 7, 2019

⬆️ jest 24.3.0

ae54ea2

Should fix the jest failure on master. * nodejs/node#25963 (comment) * jestjs/jest#8069

nikolas added a commit to ccnmtl/econplayground.js that referenced this pull request Mar 7, 2019

⬆️ jest 24.3.0

c6900f6

* nodejs/node#25963 (comment) * jestjs/jest#8069

nikolas mentioned this pull request Mar 7, 2019

⬆️ jest 24.3.0 ccnmtl/econplayground.js#478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: fixup process Proxy to handle toString correctly #25963

esm: fixup process Proxy to handle toString correctly #25963

guybedford commented Feb 6, 2019

nodejs-github-bot commented Feb 6, 2019

bmeck commented Feb 6, 2019

joyeecheung commented Feb 7, 2019

devsnek commented Feb 7, 2019

zenparsing commented Feb 7, 2019 •

edited

Loading

jdalton left a comment •

edited

Loading

guybedford commented Feb 17, 2019

guybedford commented Feb 17, 2019

guybedford commented Feb 17, 2019

devsnek commented Feb 17, 2019

guybedford commented Feb 17, 2019

danbev commented Feb 18, 2019

guybedford commented Feb 22, 2019

jasnell commented Feb 22, 2019

guybedford commented Mar 3, 2019

guybedford commented Mar 3, 2019

SimenB commented Mar 7, 2019

guybedford commented Mar 7, 2019 via email

SimenB commented Mar 7, 2019

targos commented Mar 7, 2019 •

edited

Loading

targos commented Mar 7, 2019

SimenB commented Mar 7, 2019

esm: fixup process Proxy to handle toString correctly #25963

esm: fixup process Proxy to handle toString correctly #25963

Conversation

guybedford commented Feb 6, 2019

Checklist

nodejs-github-bot commented Feb 6, 2019

bmeck commented Feb 6, 2019

joyeecheung commented Feb 7, 2019

devsnek commented Feb 7, 2019

zenparsing commented Feb 7, 2019 • edited Loading

jdalton left a comment • edited Loading

Choose a reason for hiding this comment

guybedford commented Feb 17, 2019

guybedford commented Feb 17, 2019

guybedford commented Feb 17, 2019

devsnek commented Feb 17, 2019

guybedford commented Feb 17, 2019

danbev commented Feb 18, 2019

guybedford commented Feb 22, 2019

jasnell commented Feb 22, 2019

guybedford commented Mar 3, 2019

guybedford commented Mar 3, 2019

SimenB commented Mar 7, 2019

guybedford commented Mar 7, 2019 via email

SimenB commented Mar 7, 2019

targos commented Mar 7, 2019 • edited Loading

targos commented Mar 7, 2019

SimenB commented Mar 7, 2019

zenparsing commented Feb 7, 2019 •

edited

Loading

jdalton left a comment •

edited

Loading

targos commented Mar 7, 2019 •

edited

Loading