iframe-proxy

MylesBorins · 2017-10-31T06:03:28Z

The original commit was landed without running CITGM. Unfortunately
this change breaks the module d which has over 500k downloads a day.

It is worth mentioning that the compatibility hack can be removed
without breaking anything.

We should definitely revisit for the next Semver-Major but shipping
this today will cause non trivial ecosystem breakages.

Refs: #3384

The original commit was landed without running CITGM. Unfortunately this change breaks the module `d` which has over 500k downloads a day. It is worth mentioning that the compatibility hack can be removed without breaking anything. We should definitely revisit for the next Semver-Major but shipping this today will cause non trivial ecosystem breakages. Refs: nodejs#3384

MylesBorins · 2017-10-31T06:05:28Z

targos · 2017-10-31T06:07:20Z

can you post the citgm failure here please?

MylesBorins · 2017-10-31T06:07:48Z

here is an example of one of the failures

module.js:520
    throw err;
    ^

Error: Cannot find module 'd/lazy'
    at Function.Module._resolveFilename (module.js:518:15)
    at Function.Module._load (module.js:448:25)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/memoizee/lib/methods.js:6:21)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/memoizee/methods.js:3:18)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/cli-color/bare.js:10:22)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/cli-color/index.js:5:42)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/tad/lib/console.js:13:17)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/tad/index.js:8:20)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/tad/bin/tad:22:13)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Function.Module.runMain (module.js:658:10)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:613:3
npm ERR! Test failed.  See above for more details.

targos · 2017-10-31T06:10:57Z

thanks. I'm surprised we don't test this use of require. It is common to do that with lodash.

MylesBorins · 2017-10-31T06:33:50Z

@targos it seems like this commit specifically broke the case of requiring specific files in modules that only have a single character as a name. This will thankfully be limited to 26 cases! Hopefully this hint can help us with maybe landing this in the future

jasnell · 2017-10-31T14:10:27Z

CI failed completely on windows. Trying again https://ci.nodejs.org/job/node-test-commit-windows-fanned/13041/

jasnell · 2017-10-31T14:28:09Z

@MylesBorins ...

it seems like this commit specifically broke the case of requiring specific files in modules that only have a single character as a name.

A regression test for specifically this case would be helpful as a separate PR

jdalton · 2017-10-31T14:38:50Z

I don't think my implementation has the regression. I can import lazy from "d/lazy" at least.
(might be useful as a reference when fixing later)

Ah, nervermind. When forcing it on (was some semver -rc detection wonkiness) I get errors too 😢

Error [ERR_MISSING_MODULE]: Cannot find module d/lazy

jasnell · 2017-10-31T14:59:20Z

Trying CI on Windows again now that #16639 is landed: https://ci.nodejs.org/job/node-test-commit-windows-fanned/13045/ ... that failed, but I may have restarted it wrong... trying again https://ci.nodejs.org/job/node-test-pull-request/11117/

jasnell · 2017-10-31T15:53:41Z

got past the compile error on windows... should be good but letting the CI finish

MylesBorins · 2017-10-31T16:03:58Z

single windows failures parallel/test-async-wrap-uncaughtexception

edit: make that two sequential/test-inspector-async-call-stack

jasnell · 2017-10-31T16:08:38Z

Both of which appear to be flaky

jasnell · 2017-10-31T16:11:42Z

Just waiting on AIX and OSX to finish... then will get this landed

The original commit was landed without running CITGM. Unfortunately this change breaks the module `d` which has over 500k downloads a day. It is worth mentioning that the compatibility hack can be removed without breaking anything. We should definitely revisit for the next Semver-Major but shipping this today will cause non trivial ecosystem breakages. Refs: #3384 PR-URL: #16634 Refs: #3384 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2017-10-31T16:35:33Z

Landed in 45873d2

jdalton · 2017-10-31T17:24:27Z

+      request.charCodeAt(0) !== 46/*.*/ ||
      (request.charCodeAt(1) !== 46/*.*/ &&
       request.charCodeAt(1) !== 47/*/*/)) {
    var paths = modulePaths;


The use of charCodeAt jazz really complicates the logic. It was introduced in a pass to optimize lots of things but I suspect it was micro-opts carried a bit too far. Anyways, would this do to fix it:

if (! (request.length === 1 && request.charCodeAt(0) === codeOfDot) && ! (request.charCodeAt(0) === codeOfDot && (request.charCodeAt(1) === codeOfDot || request.charCodeAt(1) === codeOfSlash))) {

MylesBorins · 2017-10-31T19:23:58Z

@jdalton do you want to take a pass at a fix + test in a new PR?

jdalton · 2017-10-31T20:07:48Z

The original commit was landed without running CITGM. Unfortunately this change breaks the module `d` which has over 500k downloads a day. It is worth mentioning that the compatibility hack can be removed without breaking anything. We should definitely revisit for the next Semver-Major but shipping this today will cause non trivial ecosystem breakages. Refs: nodejs/node#3384 PR-URL: nodejs/node#16634 Refs: nodejs/node#3384 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 31, 2017

MylesBorins requested a review from jasnell October 31, 2017 06:47

jasnell approved these changes Oct 31, 2017

View reviewed changes

MylesBorins mentioned this pull request Oct 31, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

apapirovski approved these changes Oct 31, 2017

View reviewed changes

cjihrig approved these changes Oct 31, 2017

View reviewed changes

jasnell closed this Oct 31, 2017

jdalton reviewed Oct 31, 2017

View reviewed changes

MylesBorins added the dont-land-on-v4.x label Nov 14, 2017

MylesBorins added dont-land-on-v6.x labels Nov 14, 2017

MylesBorins deleted the revert-3384 branch November 14, 2017 17:44

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

MylesBorins commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

targos commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

targos commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

jasnell commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jdalton commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jdalton Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 31, 2017 via email

Uh oh!

jdalton commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jasnell commented Oct 31, 2017 •

edited

Loading

jdalton commented Oct 31, 2017 •

edited

Loading

jasnell commented Oct 31, 2017 •

edited

Loading

MylesBorins commented Oct 31, 2017 •

edited

Loading

jdalton Oct 31, 2017 •

edited

Loading

jdalton commented Oct 31, 2017 •

edited

Loading