Provide options.scriptExtensions for 2.x by supasate · Pull Request #242 · strongloop/loopback-boot

supasate · 2017-03-23T18:17:07Z

Description

Provide scriptExtensions option for custom loading.
Due to require.extensions is not used by some framework (e.g. jest). So, this feature allows specifying file extensions supported by custom loaders.

Related issues

strongloop/loopback#3204

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-03-23T18:17:09Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2017-03-23T18:17:14Z

Can one of the admins verify this patch?

slnode · 2017-03-23T18:17:14Z

Can one of the admins verify this patch?

slnode · 2017-03-23T18:17:14Z

Can one of the admins verify this patch?

bajtos · 2017-03-24T08:33:55Z

@supasate thank you for the pull request! Does @clarkorz's #240 (comment) apply here too?

The third param onlyScriptsExportingFunction cause .js files of models will not be loaded.
Line 142 in utils.js checking typeof require.extensions['.js'] === 'function' which will be false when using jest.

bajtos · 2017-03-24T08:34:01Z

@slnode ok to test

bajtos

The patch looks good in general, I'd like you to clean up the code a bit - please take a look at my comments below.

bajtos · 2017-03-24T09:34:11Z


  var appRootDir = options.appRootDir = options.appRootDir || process.cwd();
  var env = options.env || process.env.NODE_ENV || 'development';
+  var scriptExtensions = options.scriptExtensions = options.scriptExtensions ||


Modifying input arguments (setting options.scriptExtensions) is a bad practice, we have been bitten by this in the past. Please treat options are readonly/frozen.

I see that you are accessing options.scriptExtensions at https://github.com/strongloop/loopback-boot/pull/242/files#diff-ee97c5091b89979aace94674818996baR722.

Here are two of many possible solutions how to keep options frozen and still have scriptExtensions set:

// use prototypal inheritance options = Object.create(options); // shallow-copy properties options = util._extend({}, options);

(A line like the above would go to L38-39 above, before we start processing the options object.)

bajtos · 2017-03-24T09:37:37Z

+    modelDefinitions, scriptExtensions) {
+  var registry = verifyModelDefinitions(rootDir, modelDefinitions,
+                  scriptExtensions) ||
+                  findModelDefinitions(rootDir, sources, scriptExtensions);


The indentation makes it difficult to distinguish function parameters from the second function call. I am proposing the following style:

var registry = verifyModelDefinitions(rootDir, modelDefinitions, scriptExtensions); if (!registry) { registry = findModelDefinitions(rootDir, sources, scriptExtensions); }

bajtos · 2017-03-24T09:47:20Z

  var appRootDir = options.appRootDir = options.appRootDir || process.cwd();
  var env = options.env || process.env.NODE_ENV || 'development';
+  var scriptExtensions = options.scriptExtensions = options.scriptExtensions ||
+                                                    _.keys(require.extensions);


Considering that you are converting this array back into an object down in isPreferredExtension, I think it's better to make the conversion right here.

var scriptExtensions = options.scriptExtensions ? arrayToObject(scriptExtensions) : require.extensions;

bajtos · 2017-03-24T09:48:28Z

+
+function isPreferredExtension(filename, extensions) {
+  var includeExtensions = extensions ?
+    arrayToObject(extensions) : require.extensions;


Since isPreferredExtension is a private method, there is no need to keep support for the old one-arg API.

function isPreferredExtension(filename, includeExtensions) { assert(!!includeExtensions, '"includeExtensions" argument is required'); // the code below remains unchanged var ext = path.extname(filename); return (ext in includeExtensions) && !(ext in getExcludedExtensions());}

bajtos · 2017-03-24T09:55:16Z

+  process.nextTick(function() {
+    process.bootFlags.push('customFinished');
+    callback();
+  });


This is unnecessarily complex. The thing to verify in your test is whether loopback-boot recognises a custom extension. There is no need to verify how boot script are loaded/executed again.

'use strict'; module.exports = function(app, callback) { process.bootFlags.push('customjs'); callback(); };

Same comment applies to custom.customjs2.

bajtos · 2017-03-24T09:56:23Z

      });
    });

+    it('should load scripts specified in options.scriptExtensions',


it('searches boot file extensions specified in options.scriptExtensions')

See also http://loopback.io/doc/en/contrib/style-guide.html#test-naming

bajtos · 2017-03-24T09:59:52Z

@supasate also please take a look at #240 (comment) - please add a test reproducing the issue (I think the trick will be to use a model file with a custom extension that's not registered in required.extensions.)

bajtos · 2017-03-24T11:22:48Z

Also please rebase your patch on top of the latest 2.x branch, I fixed the failing Node 0.10 builds in #244.

supasate · 2017-03-30T06:14:34Z

Hi @bajtos, I've rebased and fixed issues based on your comments. I made a sample repo at https://github.com/supasate/loopback-boot-with-jest. I'll squash later if it's alright. I also fixed the 3.x branch.

supasate · 2017-03-30T06:27:52Z

I also found another issue about file name resolving (strongloop/loopback#2201) (not related to this PR but it will make jest failed to run).

bajtos · 2017-03-31T14:13:30Z

 }

-function resolveAppScriptPath(rootDir, relativePath, resolveOptions) {
+function resolveAppScriptPath(rootDir, relativePath, resolveOptions,


Please keep resolveOptions as the last function arg. I would consider converting scriptExtensions into a property in resolveOptions, to keep the number of function arguments reasonably short.

After refactoring, there is no need to use scriptExtensions in this function anymore.

bajtos · 2017-03-31T14:14:44Z

+                                   scriptExtensions, normalization,
+                                   modelInstructions) {
  // load mixins from `options.mixins`
-  var sourceFiles = options.mixins || [];


What is the rationale for this change? I mean why to pass options properties as individual arguments? buildAllMixinInstructions is already accepting way too many arguments, we should look into ways how to make that list shorter, not longer...

I tried to make code look consistent with how mixinDirs and mixinSources are declared and passed to the function..

Do you want to refactor those arguments by passing only options and set their default values in buildAllMixinInstructions instead?

I refactored by passing only options.

bajtos · 2017-03-31T14:19:14Z

-          (typeof require.extensions[otherFileExtension]) === 'function') {
-          results.push(otherFile);
-        }
+        // if (!onlyScriptsExportingFunction)


We don't keep commented-out code around, please remove.

If I understand this code correctly, the intention was:

return a file with any extension when onlyScriptsExportingFunction is not set

return a file with an extension that can be loaded by Node.js runtime (an extension that is registered in require.extensions) when onlyScriptsExportingFunction is set

We should preserve these two modes.

I am proposing to replace onlyScriptsExportingFunction arg with scriptExtensions and modify all function callers accordingly.

// implementation here if (!scriptExtensions || otherFileExtension in scriptExtensions) results.push(otherFile); // usage - flag not set fixFileExtension(filepath, files) // usage -flag set fixFileExtension(filepath, files, scriptExtensions)

Oops! That's my mistake. I tested something and forgot to uncomment. I'll fix as you suggested.

bajtos · 2017-03-31T14:26:30Z

@supasate thank you for the pull request, please take a look at the comment I left above. Also please rebase your patch on top of the latest 2.x branch.

supasate · 2017-03-31T17:10:22Z

About rebasing on top of the latest 2.x branch, do you mean latest of 2.x branch or 2.x-latest branch? (I've rebased with 2.x branch because it seems its last commit is more up-to-date than 2.x-latest.) A little bit confuse with the branch names.

bajtos

LGTM 👍

bajtos · 2017-04-03T13:13:56Z

Landed, thank you for the contribution!

bajtos · 2017-04-03T13:15:06Z

Released in loopback-boot@2.24.0 🎉

supasate · 2017-04-03T13:34:08Z

Thanks @bajtos !

I think you can review the 3.x branch also. The code is almost the same except that it is splitted into several files.

supasate mentioned this pull request Mar 23, 2017

Provide scriptExtensions option for 3.x #240

Merged

2 tasks

bajtos self-assigned this Mar 24, 2017

bajtos self-requested a review March 24, 2017 08:34

bajtos added the feature label Mar 24, 2017

bajtos suggested changes Mar 24, 2017

View reviewed changes

bajtos suggested changes Mar 31, 2017

View reviewed changes

Provide options.scriptExtensions

95e5201

supasate force-pushed the 2.x branch from 49de2d4 to 95e5201 Compare March 31, 2017 19:10

bajtos approved these changes Apr 3, 2017

View reviewed changes

bajtos merged commit 440a75d into strongloop:2.x Apr 3, 2017

Conversation

supasate commented Mar 23, 2017

Description

Related issues

Checklist

Uh oh!

slnode commented Mar 23, 2017

Uh oh!

slnode commented Mar 23, 2017

Uh oh!

slnode commented Mar 23, 2017

Uh oh!

slnode commented Mar 23, 2017

Uh oh!

bajtos commented Mar 24, 2017

Uh oh!

bajtos commented Mar 24, 2017

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Mar 24, 2017

Uh oh!

bajtos commented Mar 24, 2017

Uh oh!

supasate commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supasate commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Mar 31, 2017

Uh oh!

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

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 3, 2017

Uh oh!

bajtos commented Apr 3, 2017

Uh oh!

supasate commented Apr 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

supasate commented Mar 30, 2017 •

edited

Loading

supasate commented Mar 31, 2017 •

edited

Loading