Skip to content

Ignore js sourcemap files from boot#263

Merged
bajtos merged 1 commit intostrongloop:2.xfrom
NextFaze:fix/ignore-maps
Sep 15, 2017
Merged

Ignore js sourcemap files from boot#263
bajtos merged 1 commit intostrongloop:2.xfrom
NextFaze:fix/ignore-maps

Conversation

@zbarbuto
Copy link
Copy Markdown
Member

@zbarbuto zbarbuto commented Sep 14, 2017

Description

See discussion from #256 and #245

Related issues

Checklist

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

cc @bajtos

@slnode
Copy link
Copy Markdown

slnode commented Sep 14, 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 Sep 14, 2017

Can one of the admins verify this patch?

2 similar comments
@slnode
Copy link
Copy Markdown

slnode commented Sep 14, 2017

Can one of the admins verify this patch?

@slnode
Copy link
Copy Markdown

slnode commented Sep 14, 2017

Can one of the admins verify this patch?

@zbarbuto zbarbuto mentioned this pull request Sep 14, 2017
2 tasks
@lehni
Copy link
Copy Markdown
Contributor

lehni commented Sep 14, 2017

I haven't been part of the previous discussions in #256, but this does look good to me. Let's see what @bajtos thinks, since he pointed out some problems in that issue.

@zbarbuto don't forget to sign the CLA, as otherwise the checks won't be completed and we won't be able to merge.

@zbarbuto
Copy link
Copy Markdown
Member Author

Whoops. Forget I need to sign it for each individual SL repo I contribute to.

@bajtos
Copy link
Copy Markdown
Member

bajtos commented Sep 14, 2017

@slnode ok to test

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.

Great, thank you @zbarbuto for the pull request 👍 Please see my two comments below for things I'd like you to improve before this can be landed.

function getExcludedExtensions() {
return {
'.json': '.json',
'.map': '.map',
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.

Please add a comment explaining that this is a temporary workaround - see #245 (comment)

I think the .map entry should be also accompanied by a comment explaining that it's a partial fix for a bigger problem and pointing to #245 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, yep, missed that. Cheers.


expect(instructions.middleware.middleware[0]).have.property(
'sourceFile', middleware);
expect(instructions.middleware.middleware[0]).not.have.property(
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 second expectation is redundant - if the sourceFile property is set to middleware, then the second expectation will always pass. If sourceFile is not set to middleware, then the first expect will abort the test and this second expectation won't be run at all.

Please remove these two lines (2206-2207).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, yes, fair point. My goal was to ensure that the map file is definitely never loaded but I missed the index being used. Will remove.

@bajtos bajtos mentioned this pull request Sep 14, 2017
2 tasks
@bajtos bajtos self-assigned this Sep 14, 2017
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.

LGTM 👍

@bajtos bajtos merged commit 35e6683 into strongloop:2.x Sep 15, 2017
@bajtos
Copy link
Copy Markdown
Member

bajtos commented Sep 15, 2017

Released in loopback-boot@2.26.2, thank you for the contribution!

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.

4 participants