node: Update strings node.js -> bootstrap_node.js

[joshgav]

Pull Request check-list

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to [CONTRIBUTING.md][0]?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?

Affected core subsystem(s)

node.cc

Description of change

Update filename in node.cc and tests to bootstrap_node.js per #5103 and update nearby comments. Fixed some other comments while I was at it.

Will need to rebase once #5881 lands.

[evanlucas]

/cc @Fishrock123

[Fishrock123]

curious that the message tests pass without this change?

[cjihrig]

Guessing it was ExecuteString() setting the script name to node.js.

[joshgav]

Yes, ExecuteString calls v8::Script::Compile which allows us to specify the ScriptOrigin, which comes from the FIXED_ONE_BYTE_STRING I changed. grep ExecuteString ./src/* -r says it isn't called anywhere else, perhaps ExecuteString should be merged into LoadEnvironment?

[Fishrock123]

These commits need to be separate PRs.

[Fishrock123]

I'm also pretty sure this is a breaking change, cc @nodejs/ctc

Is this 100% necessary, even if previously it was now not technically correct?

[cjihrig]

@joshgav was the set_trace_sync_io part causing any problems anywhere? If so, can you provide a test case?

[joshgav]

moved --trace-sync-io fix to its own PR #5964, will provide a test there

[joshgav]

@Fishrock123 would internal/bootstrap_node.js be better than just bootstrap_node.js? It seems to have the same effect, might as well get it right now. IMO the reason to fix this is to help people when debugging, so the internal prefix would probably be best.

[Fishrock123]

cc @nodejs/ctc

I think leaving it alone would probably be o.k., but if it should be changed that's fine.

It shouldn't contain the path though, no internal/, it's just representative of the filename. Any file search will pull it out pretty easily. (searching for node.js shows boostrap_node.js in my editor' file finder)

[cjihrig]

Is the question whether or not internal/ should appear in the stack traces? If so, I think it should be in there.

[joshgav]

The question is two-fold:

1. Do we leave the traced script name as is - i.e. node.js - and somehow document that the actual script is at ./lib/internal/bootstrap_node.js.
2. If we change the traced script name, do we use internal/bootstrap_node.js or bootstrap_node.js.

Personally I prefer including internal/ cause it helps with following the traces.

@Fishrock123 Do we have the option of perhaps renaming as internal/node.js (i.e. remove bootstrap_)? That might make option 1 above more palatable.

[cjihrig]

IMO we should update it to most accurately reflect reality if possible.

[Fishrock123]

Is this only exposed to error logs? I am relatively concerned we might be breaking something because I'm not entirely sure what this changes.

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

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/177/

[joshgav]

I moved this fix to another branch in my fork; if we want to continue the discussion let me know and I'll open a corresponding new PR. Thanks!

[Fishrock123]

Oh shoot

I guess? we should do this but I'm not the best person to ask about that