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

Remove public util use in lib/vm.js and lib/zlib.js #26716

Closed
wants to merge 16 commits into from

Conversation

digitaldina
Copy link

Removed public use of util in lib/vm.js and lib/zlib.js

Checklist

@nodejs-github-bot nodejs-github-bot added vm Issues and PRs related to the vm subsystem. zlib Issues and PRs related to the zlib subsystem. labels Mar 17, 2019
lib/vm.js Outdated Show resolved Hide resolved
lib/zlib.js Outdated Show resolved Hide resolved
@tniessen
Copy link
Member

Welcome @dinaelhanan and thank you for your contribution. Your changes to zlib.js cause the following error:

zlib.js:35
    isAnyArrayBuffer,
    ^
TypeError: Cannot destructure property `isAnyArrayBuffer` of 'undefined' or 'null'.
    at zlib.js:34:10
    at NativeModule.compile (internal/bootstrap/loaders.js:302:5)
    at Function.nativeModuleRequire [as require] (internal/bootstrap/loaders.js:204:7)
    at Function.Module._load (internal/modules/cjs/loader.js:591:25)
    at Module.require (internal/modules/cjs/loader.js:719:19)
    at require (internal/modules/cjs/helpers.js:14:16)
    at Object.<anonymous> (/home/travis/build/nodejs/node/deps/npm/node_modules/node-fetch-npm/src/index.js:12:14)
    at Module._compile (internal/modules/cjs/loader.js:813:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:824:10)
    at Module.load (internal/modules/cjs/loader.js:680:32)

You can find information about building and testing node locally here. Let us know if you need assistance.

@digitaldina
Copy link
Author

@ZYSzys any clues why I'm getting this error? appreciate the help :)

lib/vm.js Outdated Show resolved Hide resolved
lib/zlib.js Outdated Show resolved Hide resolved
lib/zlib.js Outdated Show resolved Hide resolved
@digitaldina
Copy link
Author

@ZYSzys hey, I've made few changes but I didn't touch isAnyArrayBuffer, however, I'm getting errors on it. Any ideas why?

@BridgeAR
Copy link
Member

@dinaelhanan the reason for it is a cyclic structure. The former loading order allowed the module to properly resolve. Some changes reference them self, so one module returns undefined. It's important to load modules in the correct order and not every module can just plainly be replaced.

@BridgeAR
Copy link
Member

@digitaldina
Copy link
Author

@BridgeAR ahhhh makes sense!

lib/zlib.js Outdated Show resolved Hide resolved
@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Mar 25, 2019
Co-Authored-By: dinaelhanan <dinaelhanan@gmail.com>
@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Mar 27, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

lib/zlib.js Show resolved Hide resolved
lib/zlib.js Outdated Show resolved Hide resolved
@digitaldina
Copy link
Author

@ZYSzys should be good but it's still showing an error :/

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

BridgeAR commented Apr 4, 2019

@dinaelhanan it turns out that after a lot of similar PRs like this one landed that util is not loaded internally anymore by default (which is great and the actual goal behind all this).

AssertionError [ERR_ASSERTION]: These modules were not loaded:
  NativeModule util

    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-bootstrap-modules.js:119:8)

Would you be so kind and update the test as well so this can land? :)

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2019
@digitaldina
Copy link
Author

digitaldina commented Apr 5, 2019

@BridgeAR done, let me know if there's anything I need to fix 👍

@nodejs-github-bot
Copy link
Collaborator

@ZYSzys
Copy link
Member

ZYSzys commented Apr 8, 2019

@dinaelhanan It seems that there is still something wrong in test-bootstrap-modules.js, it would be great if you can investigate and fix it :)

(Maybe it would be less painful if you can do a git rebase firstly)

It's weird that our travis CI passed and didn't catch that error before :D

@BridgeAR
Copy link
Member

@dinaelhanan sorry that this did not go better but please have another look. Thanks :)

@richardlau
Copy link
Member

@dinaelhanan It seems that there is still something wrong in test-bootstrap-modules.js, it would be great if you can investigate and fix it :)

(Maybe it would be less painful if you can do a git rebase firstly)

It's weird that our travis CI passed and didn't catch that error before :D

It was a bug that was introduced into the Travis scripts that has been fixed: #27176
The test did fail in Travis but the aformentioned bug meant the job status didn't fail: https://travis-ci.com/nodejs/node/jobs/190734809#L1161-L1181

@BridgeAR
Copy link
Member

This seems outdated and the specific changes have already landed else wise.

@dinaelhanan thank you very much for your contribution and I am sorry that this could not land! Please feel free to see if there's not something else for you that you would like to contribute with instead!

@BridgeAR BridgeAR closed this Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants