Skip to content
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.

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

Import file with dots in file name #3453

Merged
merged 3 commits into from
Feb 9, 2020

Conversation

life777
Copy link
Contributor

@life777 life777 commented Nov 28, 2019

Fixes #3288
This PR consist of three parts: importing files in node, importing files in browser, and unit test.
Fix described in issue isn't full because if file has dots in name it doesn't mean that we shouldn't add extension. That's why we should try to load file with and without extension.

As an example:

  1. hello.css - this file has extension and we shouldn't add .less at the end.
  2. hello.world - this file doesn't have extension and we probably should add .less at the end.

Thanks a lot.

@matthew-dean
Copy link
Member

🤔 Thanks for the PR! This looks decent... but I'd like to see more tests to make sure this doesn't add any false positives, including:

  • How .css.less files, or .less.css files are handled. Is this addressed? Your statement above says that .css always is assumed to be the end of the extension. Is that a fair assumption?
  • Does this cover other edge cases where .css or .less might be part of the filename or URL?
  • Will this unnecessarily expand the number of typical XHR requests in a browser environment? Or are these other options only tried when the most likely result fails?

@life777
Copy link
Contributor Author

life777 commented Dec 2, 2019

@matthew-dean
Thanks for your feedback.
Here it how it works:

  1. It checks whether file name ends with right extension. If yes than it starts importing this file (like css.less or css.js for plugins)
  2. Then it checks if there are dots in file name (like _nav). If no than
  • for node it will try with and without extension (_nav and _nav.less);
  • for browser it will add extension for importing (_nav.less)
  1. If file has dots in name (like less.css) than we will try to import with and without extension for both cases: less.css.less and less.css

Ok, let's answer your concerns:

  1. .css.less - will be requests as it is; .less.css - will try requeste .less.css.less, then .less.css
  2. Sure, it matches extensions only if file name ends with certain extension.
  3. Mostly - no because if file has extension it will request only one file with this extension. if file doesn't end with certain extension than in most cases it should be added and will be first options. I see only one valid case where there will be two requests instead of one: hello.css. Because in options there is a ".less" extension and it expects to find it at the end. As far as there is no such extension first option will be hello.css.less, second hello.css.

Thanks

@matthew-dean matthew-dean merged commit fd66e44 into less:master Feb 9, 2020
@matthew-dean
Copy link
Member

Looks good, thanks again!

@matthew-dean
Copy link
Member

@life777

Looks like this might have caused the following issues: #3472 #3473 #3474

With the amount of knock-on bugs this appears to have caused, I'll either revert this PR end-of-day unless you have a fix available.

matthew-dean pushed a commit that referenced this pull request Feb 10, 2020
@matthew-dean
Copy link
Member

Reverted in #3475

matthew-dean added a commit that referenced this pull request Feb 11, 2020
* Include tslib dependency
* Revert #3453
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.

@import: try .less extension even when there's a dot (.) in the filename
2 participants