Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Ignore invalid paths #5

Merged
merged 3 commits into from
Jul 11, 2019
Merged

Conversation

achlee
Copy link

@achlee achlee commented Jul 7, 2019

Packages can produce invalid relative paths for source maps. Don't error out when this happens, but rather exclude them from the dump.

This is from including the popper.js package:

        "js/../../src/utils/debounce.js?",
        "js/../../src/utils/isBrowser.js?",
        "js/../../src/utils/isFunction.js?",
        "js/../../src/utils/getStyleComputedProperty.js?",
        "js/../../src/utils/getParentNode.js?",
        "js/../../src/utils/getScrollParent.js?",
        "js/../../src/utils/isIE.js?",
        "js/../../src/utils/getOffsetParent.js?",
        "js/../../src/utils/getRoot.js?",
        "js/../../src/utils/findCommonOffsetParent.js?",
        "js/../../src/utils/isOffsetContainer.js?",
        "js/../../src/utils/getScroll.js?",
        "js/../../src/utils/getBordersSize.js?",
        "js/../../src/utils/getWindowSizes.js?",
        "js/../../src/utils/getClientRect.js?",
        "js/../../src/utils/getBoundingClientRect.js?",

I believe it's related to floating-ui/floating-ui#785, which has been fixed in master but not published yet. However, I don't believe we should error because of issues like this.

@coveralls
Copy link

coveralls commented Jul 7, 2019

Pull Request Test Coverage Report for Build 38

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 55.016%

Totals Coverage Status
Change from base Build 35: 0.1%
Covered Lines: 170
Relevant Lines: 309

💛 - Coveralls

$source = ltrim($source, '/');
return $source;
} catch (\Exception $e) {
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should still return the $source here. This bit of code is just attempting to make the source file names more relative to the root in the source maps (and thus, show a better directory listing in Chrome DevTools sources panel).

If the source files have crazy paths such that they seem to go above the root, should just let the path be unaffected.

The result will be Chrome DevTools either 1) "collapses" the extra path components to the root or 2) shows the extra path components as "../" or something. Which it is depends on if there is a <something>:// in the sourceRoot, which there is here, so 2) should happen. (i recently confirmed this behavior here)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should still return the $source

actually, wrap just the removeRelPathComponents in the try/catch and still ltrim the leading / in either case. i guess that does something useful (I don't remember why I had that - I guess it removes a pointless directory).

Copy link
Author

Choose a reason for hiding this comment

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

In this particular case I don't believe these are valid paths. Is there any negative side effects of including sources that are invalid? i.e. popper.js isn't actually including those files in the npm dist folder.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any negative side effects of including sources that are invalid? i.e. popper.js isn't actually including those files in the npm dist folder.

No - the filenames in sources are just used to better identify a line/column from the generated JS. There is no expectation that it is an accessible file, either from the web or at build time.

These file names came from the source map located in this popper.js node module (here's the source map they provide in the latest version). The corresponding sourcesContent array is where the original code is.

i.e. popper.js isn't actually including those files in the npm dist folder.

That's OK - a package should distribute source maps along with their bundled files, but they don't also need to distribute the original source files.

I'd expect that popper.js is included directly in the SourceMapFilter usage (not indirectly via another JS bundle) - npm would only be relevant during a build step like webpack does, and if that were the case the source map popper.js distributes wouldn't be used, as the build system would just generate that information from the source files itself. Anyway, just sorta guessing here, would have see the usage to make a more intelligible description of what's going on :)

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

lgtm!

@achlee
Copy link
Author

achlee commented Jul 11, 2019

Thanks @connorjclark !

@Chikashi-Kato Chikashi-Kato merged commit 628b97f into coursehero:master Jul 11, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants