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

feat(package): support less >= v3.0.0 #242

Merged
merged 1 commit into from
Mar 9, 2018
Merged

feat(package): support less >= v3.0.0 #242

merged 1 commit into from
Mar 9, 2018

Conversation

thorn0
Copy link
Contributor

@thorn0 thorn0 commented Feb 18, 2018

@jsf-clabot
Copy link

jsf-clabot commented Feb 18, 2018

CLA assistant check
All committers have signed the CLA.

@alexander-akait
Copy link
Member

@thorn0 look very good, thanks!

@thorn0
Copy link
Contributor Author

thorn0 commented Feb 19, 2018

For the full test coverage we'll have to run tests with both Less 2.x and 3.x. Because of this:

+        less.version[0] >= 3
+          ? options.ext && !tildePrefixedModuleName.test(filename)
+            ? this.tryAppendExtension(filename, options.ext)
+            : filename
+          : filename.replace(matchMalformedModuleFilename, '$1');

@alexander-akait
Copy link
Member

@thorn0 can you do this?

@thorn0
Copy link
Contributor Author

thorn0 commented Feb 19, 2018

Also the tests sometimes fail now because of the race condition in Less: less/less.js#3170

@alexander-akait
Copy link
Member

@thorn0 i think we can fix this in other PR, but if solution is simple you can do this here 👍

@thorn0
Copy link
Contributor Author

thorn0 commented Feb 19, 2018

Re running tests with Less 2. I can do it, but not sure when. Consider merging this as is and opening an issue.

@fraywing
Copy link

fraywing commented Feb 22, 2018

Let's submit this thing! @evilebottnawi

@rejas rejas mentioned this pull request Feb 23, 2018
@davidpanzarella
Copy link

Any chance this will get submitted soon? @evilebottnawi @jhnns @d3viant0ne @michael-ciniawsky

@michael-ciniawsky michael-ciniawsky changed the title fix: support Less 3 fix(package): support less >= v3.0.0 Feb 23, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@thorn0 Thx, to clearify is this PR backwards compatible (supports both less v2.x && v3x) (🏷 Patch) or a BREAKING CHANGE (🏷 Major) ?

// This somewhat changed in Less 3.x. Now the file name comes without the
// automatically added extension whereas the extension is passed in as `options.ext`.
// So, if the file name matches this regexp, we simply ignore the proposed extension.
const tildePrefixedModuleName = /^~[^/\\]+$/;
Copy link
Member

Choose a reason for hiding this comment

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

tildePrefixedModuleName => isModule || isModuleName || isModuleRequest

return false;
}

loadFile(filename, currentDirectory, options /* , environment */) {
Copy link
Member

Choose a reason for hiding this comment

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

Either enviroment is needed or not, but please don't leave commented stuff

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Feb 23, 2018

Seems to be a semver patch (Waiting for feedback)

@michael-ciniawsky
Copy link
Member

- "less": "^2.3.1",
+ "less": "^2.0.0 || ^3.0.0",

@thorn0
Copy link
Contributor Author

thorn0 commented Feb 23, 2018

Why not "less": "^2.3.1 || ^3.0.1"? Are you sure it'll work with say 2.0.0?

@michael-ciniawsky
Copy link
Member

Only the major version is important here imho, this avoids noisy false warnings within the specified semver major ranges, e.g would =< v2.3.1 currently not work ? If a certain minor/path version doesn't work, then this would needed to be reflected in the peerDependencies of course, but assuming this isn't the case here :)

@thorn0
Copy link
Contributor Author

thorn0 commented Feb 23, 2018

I respectfully disagree. Minor version means added features. Are you sure we don't use any features added between 2.0 and 2.3? You're not, so why change this?

@michael-ciniawsky
Copy link
Member

Sure you can keep the status quo and just add || ^3.0.0 aswell

@michael-ciniawsky
Copy link
Member

@thorn0 Thx

@fraywing
Copy link

@thorn0, go for it!

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Feb 26, 2018

@thorn0 You rightfully opened this against the next branch, the problem is that next contains breaking changes and would need to be released as a semver major. next is also not 💯 ready yet. Would it be possible to rebase this PR against master, so I can release it as a patch instead ?

@michael-ciniawsky michael-ciniawsky changed the base branch from next to master February 26, 2018 03:03
@thorn0
Copy link
Contributor Author

thorn0 commented Feb 27, 2018

@michael-ciniawsky done, pls have a look


const fixturePath = path.resolve(__dirname, '..', 'fixtures');
const outputPath = path.resolve(__dirname, '..', 'output');

function compile(fixture, moduleRules, resolveAlias = {}) {
return new Promise((resolve, reject) => {
const entry = path.resolve(fixturePath, 'less', `${fixture}.less`);

webpack({
const [majorVersion] = webpackPackageJson.version.split('.');
Copy link
Member

Choose a reason for hiding this comment

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

Please drop this, I appreciate that you want to fix it, but adding webpack >= v4.0.0 to the CI setup will be part of #233 asap. The loader works with the webpack >= v4.0.0 Loader API (no affecting breaking changes between v3.0.0 => v4.0.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? The tests don't pass without this change.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'm sure about that, please pin webpack to ^3.0.0 (https://github.com/webpack-contrib/less-loader/blob/master/package.json#L65) instead

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

One thing to drop and g2g :)

@hlehmann
Copy link

hlehmann commented Mar 7, 2018

is it good to be merged ?

@davidpanzarella
Copy link

Any update on when this will be merged?

@michael-ciniawsky michael-ciniawsky changed the title fix(package): support less >= v3.0.0 feat(package): support less >= v3.0.0 Mar 9, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@thorn0 Thx

@michael-ciniawsky michael-ciniawsky merged commit d8c9d83 into webpack-contrib:master Mar 9, 2018
@michael-ciniawsky
Copy link
Member

Released in v4.1.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants