Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

test: add test for MultiCompiler usage #74

Merged
merged 7 commits into from
Jul 9, 2017
Merged

test: add test for MultiCompiler usage #74

merged 7 commits into from
Jul 9, 2017

Conversation

hulkish
Copy link
Contributor

@hulkish hulkish commented Jul 8, 2017 •
edited
Loading

Fixes #73

Update: So, it looks like this issue just magically fixed itself. That, said - I'd like to still include the added tests and upgrade to uglify-es@3.0.24.

@hulkish hulkish force-pushed the hulkish-FixMultiCompiler branch 4 times, most recently from fc62311 to f9b569b Compare July 8, 2017 03:48
Jessidhia
Jessidhia reviewed Jul 8, 2017
View reviewed changes
test/helpers.js Outdated
const eventLength = Array.isArray(_plugins[name]) ? _plugins[name].length : 0;

if (eventLength > currentLength) {
Object.assign(aggregate, {
Copy link

Choose a reason for hiding this comment

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

Why not just aggregate[name] = eventLength?

test/helpers.js

exports.removeCWD = function removeCWD(str) {
export function countPlugins({ _plugins }) {
Copy link

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Member

Choose a reason for hiding this comment

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

What is the usecase here ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kovensky @michael-ciniawsky Was for the test I hadn't added at first to this pr. But, I have since added it: https://github.com/webpack-contrib/uglifyjs-webpack-plugin/pull/74/files#diff-0570d5214a7cbff246a7ad90d119f15c

@joshwiens joshwiens changed the title Fix MultiCompiler support fix: MultiCompiler support Jul 8, 2017
michael-ciniawsky
michael-ciniawsky reviewed Jul 8, 2017
View reviewed changes
src/index.js Outdated
@@ -149,7 +149,7 @@ class UglifyJsPlugin {
}

apply(compiler) {
const requestShortener = new RequestShortener(compiler.context);
const requestShortener = new RequestShortener(compiler.context || process.cwd());
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jul 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is process.cwd() save to use in most cases ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, i have a regression test to add that uses this

Copy link
Member

Choose a reason for hiding this comment

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

@michael-ciniawsky - Agree here. We should get context without using process. || window || w/e

test/helpers.js

exports.removeCWD = function removeCWD(str) {
export function countPlugins({ _plugins }) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the usecase here ? :)

@michael-ciniawsky michael-ciniawsky changed the title fix: MultiCompiler support fix: MultiCompiler support Jul 8, 2017
@michael-ciniawsky michael-ciniawsky added this to the 1.0.0 milestone Jul 8, 2017
@hulkish
Copy link
Contributor Author

hulkish commented Jul 8, 2017

path.resolve()?

@michael-ciniawsky
Copy link
Member

path.resolve() ?

For compiler.context you mean ? If can you post what the status quo is there atm please? I wonder why it isn't absolute and if this has a reason

@hulkish hulkish force-pushed the hulkish-FixMultiCompiler branch from f9b569b to 351cbfa Compare July 8, 2017 17:29
@hulkish
Copy link
Contributor Author

hulkish commented Jul 8, 2017

@michael-ciniawsky yes, path.resolve & process.cwd() both seem to be used for this in webpack core: https://github.com/webpack/webpack/search?utf8=%E2%9C%93&q=%22new+RequestShortener%28%22&type=

@hulkish hulkish force-pushed the hulkish-FixMultiCompiler branch from 264114e to 50bf746 Compare July 8, 2017 18:15
@michael-ciniawsky michael-ciniawsky changed the title fix: MultiCompiler support test: add test for MultiCompiler usage Jul 8, 2017
@michael-ciniawsky michael-ciniawsky mentioned this pull request Jul 9, 2017
Merged
@michael-ciniawsky michael-ciniawsky merged commit d23681a into master Jul 9, 2017
@michael-ciniawsky michael-ciniawsky deleted the hulkish-FixMultiCompiler branch July 9, 2017 16:10
@michael-ciniawsky michael-ciniawsky removed this from the 2.0.0 milestone Oct 29, 2017
@snyk-bot snyk-bot mentioned this pull request Jun 2, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joshwiens joshwiens joshwiens left review comments

@Jessidhia Jessidhia Jessidhia left review comments

@michael-ciniawsky michael-ciniawsky michael-ciniawsky approved these changes

Assignees
No one assigned
Labels
semver: Patch status: Approved type: CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: uglifyjs-weback-plugin@1.0.0-beta.1 breaks with MultiCompiler
4 participants
@hulkish @michael-ciniawsky @Jessidhia @joshwiens