Skip to content

avoids trying to load middleware excluded by scriptExtensions#279

Closed
ebbnormal wants to merge 1 commit intostrongloop:2.xfrom
ebbnormal:2.x
Closed

avoids trying to load middleware excluded by scriptExtensions#279
ebbnormal wants to merge 1 commit intostrongloop:2.xfrom
ebbnormal:2.x

Conversation

@ebbnormal
Copy link
Copy Markdown

@ebbnormal ebbnormal commented Mar 27, 2018

See #246

@ebbnormal ebbnormal changed the title avoids trying to load middleware exluded by scriptExtensions avoids trying to load middleware excluded by scriptExtensions Mar 29, 2018
@bajtos
Copy link
Copy Markdown
Member

bajtos commented Apr 17, 2018

Hello @ebbnormal, thank you for the pull request. What problem are you trying to solve?

I see the pull request is linked to #246. IIUC, that issue is describing a problem that has been already fixed by #263 in loopback-boot@2.26.2.

@ebbnormal
Copy link
Copy Markdown
Author

ebbnormal commented Apr 17, 2018 via email

@bajtos
Copy link
Copy Markdown
Member

bajtos commented Apr 19, 2018

I don’t think that solution included piping in the extensions from the loop back boot options.

I am afraid I still don't understand what are you trying to solve.

Could you please add a unit-test that's failing at the current master and passing with your patch, one that demonstrates your use case?

Alternatively, it's enough if you can describe your use case in a way that allows me to reproduce it. What steps/configuration changes to make? What is the actual (observed) result? What did you expect to see instead?

@bajtos
Copy link
Copy Markdown
Member

bajtos commented Apr 19, 2018

Quoting from #246:

We tried to do a workaround by including the new options variable to boot() by adding the scriptExtensions [".js"]. We thought this would exclude the .map file being executed in the middleware directory. However, when we debugged it, we saw that the scriptExtensions was undefined at the moment of filtering out files based on extensions. We traced this back to fixFileExtensions not received the options.scriptExtensions.

I think I am starting to understand this little bit. What would be the purpose of options.scriptExtensions? To provide a list of extensions that your Node.js application can load as a source code? That list can be obtained via Object.keys(require.extensions) (see docs), why would you want to customize it?

Comment thread lib/executor.js

middleware.forEach(function(data) {
var middlewareUnexcluded = _.find(instructions.files.boot, function(o) { return o === data.sourceFile });
if (!middlewareUnexcluded){ return; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a wrong place where to filter middleware. All filtering must happen in the compiler phase, executor is only interpreting the instructions produced by the compiler.

However, when we debugged it, we saw that the scriptExtensions was undefined at the moment of filtering out files based on extensions. We traced this back to fixFileExtensions not received the options.scriptExtensions.

If fixFileExtenisons is not receiving options.scriptExtensions, then the way to go is to fix the compiler to correctly pass options.scriptExtensions all the way to fixFileExtensions when working with the middleware.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos I see, thank you for clarifying. I actually cannot replicate the issue on version 2.x.x. It looks like you guys are grabbing the extensions from options as of 2.27.x Closing for now. Thank you!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am glad the issue has been resolved for you 👍

@ebbnormal ebbnormal closed this Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants