Skip to content

Provide scriptExtensions option for 3.x#240

Merged
raymondfeng merged 1 commit intostrongloop:masterfrom
supasate:provide-script-extensions-option
Apr 3, 2017
Merged

Provide scriptExtensions option for 3.x#240
raymondfeng merged 1 commit intostrongloop:masterfrom
supasate:provide-script-extensions-option

Conversation

@supasate
Copy link
Copy Markdown
Contributor

@supasate supasate commented Mar 8, 2017

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
Copy link
Copy Markdown

slnode commented Mar 8, 2017

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

@slnode
Copy link
Copy Markdown

slnode commented Mar 8, 2017

Can one of the admins verify this patch?

2 similar comments
@slnode
Copy link
Copy Markdown

slnode commented Mar 8, 2017

Can one of the admins verify this patch?

@slnode
Copy link
Copy Markdown

slnode commented Mar 8, 2017

Can one of the admins verify this patch?

@supasate supasate mentioned this pull request Mar 8, 2017
Closed
@bajtos
Copy link
Copy Markdown
Member

bajtos commented Mar 9, 2017

Thank you @supasate for the pull request.

I am personally not very familiar with loopback-boot@3 code base, I'll defer review to @davidcheung and/or @raymondfeng.

@bajtos
Copy link
Copy Markdown
Member

bajtos commented Mar 9, 2017

@slnode ok to test

@supasate
Copy link
Copy Markdown
Contributor Author

supasate commented Mar 23, 2017

Is there any update on this PR?

@slnode
Copy link
Copy Markdown

slnode commented Mar 23, 2017

Can one of the admins verify this patch?

@bajtos
Copy link
Copy Markdown
Member

bajtos commented Mar 23, 2017

Hi @supasate, sorry for the delay. I'll ping @davidcheung and @raymondfeng to bring this pull request into their attention.

Meanwhile, if you are using loopback-boot@2 in your application, it may be better for you to submit this feature to our 2.x branch. I can review such patch myself and release it in the current 2.x release line. (master is tracking 3.0 that hasn't been released yet.)

@supasate supasate changed the title Provide scriptExtensions option Provide scriptExtensions option for 3.x Mar 23, 2017
@supasate
Copy link
Copy Markdown
Contributor Author

supasate commented Mar 23, 2017

I have made another PR for 2.x at #242.

raymondfeng
raymondfeng approved these changes Mar 23, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

LGTM

clark0x
clark0x reviewed Mar 24, 2017
View reviewed changes
Comment thread lib/plugins/model.js Outdated

// find a matching file with a supported extension like `.js` or `.coffee`
var sourceFile = fixFileExtension(jsonFile, allFiles, true);
var sourceFile = fixFileExtension(jsonFile, allFiles, true, scriptExtensions);
Copy link
Copy Markdown
Contributor

@clark0x clark0x Mar 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@bajtos bajtos Mar 24, 2017

Choose a reason for hiding this comment

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

Good catch! @supasate could you please take a look?

Copy link
Copy Markdown
Contributor Author

@supasate supasate Mar 30, 2017

Choose a reason for hiding this comment

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

@clarkorz In Jest, we will provide options.scriptExtensions = ['.js'] and it will return when it reaches if (isPreferredExtension(filepath, scriptExtensions)) return filepath; before getting into that condition.

@bajtos bajtos mentioned this pull request Mar 24, 2017
Merged
2 tasks
@supasate supasate force-pushed the provide-script-extensions-option branch from 68378cb to acef509 Compare March 30, 2017 05:58
@supasate
Copy link
Copy Markdown
Contributor Author

supasate commented Mar 30, 2017 •
edited
Loading

I've fixed the code based on @bajtos's comments on 2.x branch (will squash after reviewing in 2.x branch).

@supasate supasate force-pushed the provide-script-extensions-option branch from 7c23a1f to d68ffc6 Compare March 31, 2017 19:15
bajtos
bajtos reviewed Apr 3, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I am not familiar with 3.x/master codebase, but FWIW, the changes LGTM.

I'll leave it up to @raymondfeng or @davidcheung to give this their final approval and land it.

@raymondfeng raymondfeng merged commit 4803802 into strongloop:master Apr 3, 2017
@raymondfeng raymondfeng removed the review label Apr 3, 2017
@supasate supasate deleted the provide-script-extensions-option branch April 4, 2017 05:19
STRML added a commit to STRML/loopback-boot that referenced this pull request Feb 28, 2020
@STRML
Provide scriptExtensions option
4300b82 
Backport of strongloop#240
with some simplifications for this version
@STRML STRML mentioned this pull request Feb 28, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raymondfeng raymondfeng raymondfeng approved these changes

@davidcheung davidcheung Awaiting requested review from davidcheung

+2 more reviewers

@bajtos bajtos bajtos left review comments

@clark0x clark0x clark0x left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@supasate @slnode @bajtos @raymondfeng @clark0x @siddhipai