iframe-proxy

Kehrlann · 2018-09-17T11:39:08Z

We ran an issue with the default require('module-alias/register') because the node_modules in our prod environment was soft-linked to the current working directory.

Trying to load package.json from Node's current working directory does the trick, because we are running the app from a package manager ; so we added this as a possibility the init script will try out.

The no-argument init relies on the position of index.js in the filesystem, so it's tricky to test. And the default behavior currently has no test covering it - so we did not add a test.

Lines 147 and 148 seem redundant, you might want to have a look.

- When running the node app with a node_modules that is linked from somewhere else in the filesystem, the automatic registration `requires('module-alias/register')` breaks, because it assumes that the `module-alias` package is two levels above package.json. However, another sensible default is to assume `package.json` is in the Node process' current working directory ; this is usually true when using a package manager (npm / yarn) to launch the app. - Slightly refactored the init function so that it tries to find package.json is three directories before bailing: the user-provided path, the '../..' path and 'process.cwd()'. The last two are somewhat redundant. Signed-off-by: Sandrine Piriou <spiriou@pivotal.io>

ilearnio

Why not to detect the path before initializing module-alias and then pass it into init function?

ilearnio · 2018-09-18T07:49:35Z

ilearnio · 2018-09-18T07:49:54Z

+  for (var i in candidatePackagePath) {
+    try {
+      base = candidatePackagePath[i]
+      npmPackage = require(base + '/package.json')


Please use nodePath.join here to avoid any cross-platform issues.

npmPackage = require(nodePath.join(base, 'package.json'))

ilearnio · 2018-09-18T07:53:15Z

+  // Or that package.json is in the node process' current working directory (when
+  // running a package manager script, e.g. `yarn start` / `npm run start`)
+  var candidatePackagePath = [
+    (options.base || '').replace(/\/package\.json$/, ''),


What if options.base is not defined? Then it will add '' to the candidatePackagePath array, so it will then call require with '/package.json' as an argument.

I would change it to

var basePath = options.base ? options.base.replace(/\/package\.json$/, '') : nodePath.join(__dirname, '../..') var candidatePackagePaths = [basePath] if (basePath !== process.cwd()) { candidatePackagePaths.push(process.cwd()) }

Now that I re-read my code and your comment ; I'm thinking that if options.base is defined, we should use that and we should not even try to use process.cwd().

Thoughts ?

Actually you're right 👍

ilearnio · 2018-09-18T08:00:24Z


  if (typeof npmPackage !== 'object') {
-    throw new Error('Unable to read ' + packagePath)
+    throw new Error('Unable to read any of [' + candidatePackagePath.map(function(path) { return base + '/package.json'}).join(', ')  + ']')


base inside of map function should be changed to path, also would be good to put the map function result into it's own line.

var pathsString = candidatePackagePaths.map(function(path) { return nodePath.join(path, '/package.json') }).join('\n ') throw new Error('Unable to find package.json in:\n ' + pathsString + ']')

Kehrlann · 2018-09-19T05:45:07Z

Thanks for all the feedback. Yes for now we initialize the module manually, we thought it'd be nice if we could auto-detect.

I've changed the behavior to use options.base if defined (even if wrong); otherwise try the parent dir/process.cwd().

I've added a spec for process.cwd(), it's simple but a bit weird as I do a process.chdir().

However I have not added a spec for default settings in the parent directory. This would require importing module alias from somewhere else than the source itself... I was thinking of dynamically copying index.js to test/src/node_modules before running a test, importing that copy, testing, then deleting it. What do you think ?

- Clean the error message - Use nodePath.join instead of manual path building - First stab at process.cwd() testing, by changing the working dir of the node process in the tests

ilearnio · 2018-09-19T08:11:33Z

+
+  var candidatePackagePaths
+  if(options.base) {
+    candidatePackagePaths = [options.base.replace(/\/package\.json$/, '')]


Please wrap the options.base.replace(/\/package\.json$/, '') with nodePath.resolve, so it will fix the slashes to be cross-platform

ilearnio · 2018-09-19T08:23:31Z


  if (typeof npmPackage !== 'object') {
-    throw new Error('Unable to read ' + packagePath)
+    var pathString = candidatePackagePaths.join('\n, ')


Could you please swap \n and , in places, so it will look like .join(',\n ')

ilearnio · 2018-09-19T08:28:55Z

+  // Or that package.json is in the node process' current working directory (when
+  // running a package manager script, e.g. `yarn start` / `npm run start`)
+  var candidatePackagePath = [
+    (options.base || '').replace(/\/package\.json$/, ''),


Actually you're right 👍

- Also, fix a typo in the error log

- To check the relative path lookup, index.js must be in a very specific location. This requires copying index.js before a test and importing that. - Requires some infrastructure change in the spec.js to handle importing the regular index.js and importing the copied one ; this means we re-require the module before each test.

Kehrlann · 2018-09-22T10:51:07Z

I've taken your feedback into account and tried a few things. Some of them might be a bit bold.

We test process.cwd() ; but we do not stub it - see comment : Add default init behavior: look for package.json in process.cwd() #31 (comment)
I've added a test for relative path import (__dirname/../..), but this requires some changes in spec.js, because it means we copy index.js around ; and import it multiple times. This might be a bit too messy, let me know what you think - I can revert this commit if you prefer.

Try commenting out either nodePath.join(__dirname, '../..') or process.cwd() in index.js, each should break one test only.

Cheers !

Kehrlann · 2018-10-30T17:38:37Z

Hi Nick,

Just a bump ... Have you had a chance to review the latest changes ? Sorry to ping you here, I'd just like to avoid having this PR too outdated :)

Cheers !

ilearnio · 2018-10-30T17:49:32Z

Hey, sorry for so big delay. Very busy recently at my job since we have deadlines and a lot of stuff to do, so really not much time left for other stuff. I'll try to get my hands around it on weekend, maybe earlier

Kehrlann · 2018-10-30T17:59:59Z

Thanks for answering :) No worries, take your time, doesn't have to be this week-end !

jlsan92 · 2019-02-08T12:07:37Z

Hello! Are you planning to merge this any time soon? I'm having the same problem when using this module in Docker 😞

Kehrlann · 2019-02-10T18:28:14Z

Hey @ilearnio,

It seems you haven't had much time to work on module-alias for a while. Would you be interested in some help ? It'd require to spend a bit of time to bring me up to speed on some things, maybe review a few PRs I do, but I'd love to contribute a bit more, triage issues, work on PRs, fix some outstanding issues (as much as I can, I am a bit busy as well).

Let me know !

Again, sorry to contact you here, couldn't find another way :(

Kehrlann · 2019-02-10T18:30:20Z

@jlsan92 - take a look at : #43 (comment) ... The suggested fix might help.

If it doesn't, maybe open another issue with additional details ? e.g. your package.json, your Dockerfile, logs, etc.

ilearnio · 2019-02-10T18:43:13Z

Hey guys. Sorry for not being active in this project for a while, have crazy schedule at my work. Didn't have a normal weekend for long time.

@Kehrlann It would be really great if you can help to make it better, because I will be busy next couple of weeks for sure. I can add you as a collaborator. But please make sure that the specs are covering well every new case, especially if it's someone else's PRs. This package has more than million of installs as of now so don't want to ruin anybody's site, or my personal projects haha 😄

ilearnio · 2019-02-10T18:51:17Z

ilearnio reviewed Sep 18, 2018

View reviewed changes

ilearnio requested changes Sep 18, 2018

View reviewed changes

Kehrlann added 2 commits September 19, 2018 08:15

Do not use process.cwd() when using user-defined basePath

67af5a7

- Clean the error message - Use nodePath.join instead of manual path building - First stab at process.cwd() testing, by changing the working dir of the node process in the tests

Fix arrow function to be a regular function()

77eda05

ilearnio reviewed Sep 19, 2018

View reviewed changes

ilearnio requested changes Sep 19, 2018

View reviewed changes

Comment thread test/specs.js Outdated

Kehrlann added 3 commits September 19, 2018 16:38

Add nodepath.resolve around basepath for cross-platform compatibility

68b4f01

- Also, fix a typo in the error log

Fix linting problems

8085473

Kehrlann mentioned this pull request Dec 4, 2018

Not working in Heroku #43

Closed

ilearnio approved these changes Feb 10, 2019

View reviewed changes

Merge branch 'master' into master

3f83a50

Kehrlann merged commit 59d316d into ilearnio:master Feb 12, 2019

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

Kehrlann commented Sep 17, 2018

Uh oh!

ilearnio left a comment

Choose a reason for hiding this comment

Uh oh!

ilearnio commented Sep 18, 2018

Uh oh!

ilearnio Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

ilearnio Sep 18, 2018 • 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!

Kehrlann Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

ilearnio Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

ilearnio Sep 18, 2018 • 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!

Kehrlann commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilearnio Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

ilearnio Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

ilearnio Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kehrlann commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kehrlann commented Oct 30, 2018

Uh oh!

ilearnio commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kehrlann commented Oct 30, 2018

Uh oh!

jlsan92 commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kehrlann commented Feb 10, 2019

Uh oh!

Kehrlann commented Feb 10, 2019

Uh oh!

ilearnio commented Feb 10, 2019

Uh oh!

ilearnio commented Feb 10, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ilearnio Sep 18, 2018 •

edited

Loading

ilearnio Sep 18, 2018 •

edited

Loading

Kehrlann commented Sep 19, 2018 •

edited

Loading

Kehrlann commented Sep 22, 2018 •

edited

Loading

ilearnio commented Oct 30, 2018 •

edited

Loading

jlsan92 commented Feb 8, 2019 •

edited

Loading