Skip to content

fix: Importing files using a resource query #55

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

fix: Importing files using a resource query #55

wants to merge 2 commits into from

Conversation

@DimitarNestorov
Copy link

@DimitarNestorov DimitarNestorov commented Dec 11, 2018 •
edited
Loading

#53

@DimitarNestorov DimitarNestorov changed the title fix: Importing files using a resource query (#53) fix: Importing files using a resource query Dec 11, 2018
@smac89
Copy link

smac89 commented Apr 30, 2019

Please merge this. Very useful

smac89
smac89 reviewed Apr 30, 2019
View reviewed changes
src/extractLoader.js
__webpack_public_path__: publicPath, // eslint-disable-line camelcase
require: givenRelativePath => {
const indexOfQuery = givenRelativePath.includes("?") ? givenRelativePath.indexOf("?") : givenRelativePath.length;
const noDoubleQuestionMarkPath = givenRelativePath.replace(/\?\?/g, "##");
Copy link

@smac89 smac89 Apr 30, 2019

Choose a reason for hiding this comment

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

When is ?? part of a valid url? Also if you handled ??, why didn't you also handle ????

I don't think you need to handle these anyways. You should just focus on fixing the query part which was handled wrongly in the original code and leave these other cases to the judgement of the person using it.

Copy link
Author

@DimitarNestorov DimitarNestorov Apr 30, 2019

Choose a reason for hiding this comment

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

There was a test case failing, it was related to double question mark. I believe it is valid in webpack context.

smac89
smac89 reviewed Apr 30, 2019
View reviewed changes
src/extractLoader.js
require: givenRelativePath => {
const indexOfQuery = givenRelativePath.includes("?") ? givenRelativePath.indexOf("?") : givenRelativePath.length;
const noDoubleQuestionMarkPath = givenRelativePath.replace(/\?\?/g, "##");
const indexOfQuery = noDoubleQuestionMarkPath.includes("?") ? noDoubleQuestionMarkPath.indexOf("?") : givenRelativePath.length;
Copy link

@smac89 smac89 Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

A shorter way of accomplishing the same thing is:

const indexOfQuery = givenRelativePath.search(/(\?.*)?$/);

Copy link
Author

@DimitarNestorov DimitarNestorov Apr 30, 2019

Choose a reason for hiding this comment

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

I love it but code is meant to be readable and understandable. I'm going to leave the decision to the maintainer.

jannikkeye added a commit that referenced this pull request Jan 28, 2020
@alqh @jannikkeye
fix: handling resource paths with query parameters
b4b2c0a 
* Issue #55, #50 - Fix issue with loading resource with Query parameters, it doesn't currently slice the path for query parameters

* fix(core): support  paths with query params and loaders

* fix: linter warrnings

Co-authored-by: Jannik Keye <jannik.keye@peerigon.com>
@jannikkeye
Copy link
Contributor

jannikkeye commented Jan 28, 2020

Fixed in #66.

@jannikkeye jannikkeye closed this Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@smac89 smac89 smac89 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DimitarNestorov @smac89 @jannikkeye