-
Notifications
You must be signed in to change notification settings - Fork 33
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
Hard fork node-parent-require #1239
Conversation
…irm that all prequire is pointing to the file ./lib/parent-require.js
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1239 +/- ##
==========================================
+ Coverage 89.37% 89.39% +0.02%
==========================================
Files 23 24 +1
Lines 3321 3330 +9
==========================================
+ Hits 2968 2977 +9
Misses 353 353
☔ View full report in Codecov by Sentry. |
lib/scripts/parent-require.js
Outdated
@@ -0,0 +1,9 @@ | |||
module.exports = function (id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to lib
and credit the original author https://github.com/jaredhanson/node-parent-require
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be specific: credit him in a code comment at the top of the file
@@ -0,0 +1,9 @@ | |||
module.exports = function (id) { | |||
let parent = module.parent | |||
for (; parent; parent = parent.parent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see if it still works if you refactor it to not use module.parent
anymore, as per this comment
@@ -0,0 +1,9 @@ | |||
module.exports = function (id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget to credit the original author with a code comment at the top of the file https://github.com/jaredhanson/node-parent-require
…removing module.parent and got error, researching on other alternatives
…rmation on top of the parent-require.js file. Removed the support folder from ./lib . Ran standard --fix. Tried replacing let parent = module.parent with parent = Object.values(require.cache).filter((m) => m.children.includes(module)) but got error. Researching on a work around for now.
When starting to tackle this bug I decided to remove parent-require from package.json, causing an error Extra param "generateFolderStructure" found in rooseveltConfig, this can be removed . I added a file ../scripts/parent-require.js that is the clone of the index.js file in the node-parent-require . I also found that only two files ( setExpressConfigs.js and preprocessCss.js) were requiring it so I just pointed them to the new file in the script folder. I created a new sample app by running npx mkroosevelt and there no errors. There are tests that I still need to bring over but everything seams to be working.
Closes #1062