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

fix: only set output path on passing flag #1855

Merged
merged 5 commits into from Oct 3, 2020

Conversation

anshumanv
Copy link
Member

What kind of change does this PR introduce?

  • convert output to output-path
  • only assign output path when supplied

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
Yes

Summary
Previously we were assigning output file and path which was not correct so fixed that and converted output flag to output-path

Does this PR introduce a breaking change?
Yes

Other information
Fixes #1844

@anshumanv anshumanv requested a review from a team as a code owner October 3, 2020 06:48
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @webpack/cli-team

Good work!

@webpack-bot
Copy link

@anshumanv Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@snitin315 Please review the new changes.

@anshumanv anshumanv merged commit 2f36b9d into webpack:next Oct 3, 2020
@anshumanv anshumanv deleted the output-flag branch October 3, 2020 12:59
Rob--W added a commit to Rob--W/webextension-polyfill that referenced this pull request Oct 14, 2020
Currently the build/test fails with the following output:

```
Test webextension-polyfill bundled with webpack
===============================================
[webpack-cli] Unknown argument: --output
[webpack-cli] Unknown argument: /tmp/webpack-bundle.js
? Which flags do you want to use? …
✔ --entry: The entry point(s) of your application e.g. ./src/main.js
✔ --config: Provide path to a webpack configuration file e.g. ./webpack.config.js
✔ --color: Enable/Disable colors on console
✔ --merge: Merge two or more configurations using webpack-merge e.g. -c ./webpack.config.js -c ./webpack.test.config.js --merge
✔ --progress: Print compilation progress during build
✔ --help: Outputs list of supported flags
✔ --output-path: Output location of the file generated by webpack e.g. ./dist/
```

Regressed by:
webpack/webpack-cli#1855
webpack/webpack-cli@2f36b9d

The `-o` flag is compatible with both the old and new flag name.
@Rob--W
Copy link

Rob--W commented Oct 14, 2020

This should have been called feat: Rename --output flag to --output-path.

It is a breaking change, and quite a late one to make after beta, before RC...

@Rob--W
Copy link

Rob--W commented Oct 14, 2020

Actually, it seems that --output has been split into --output-path (=output directory) and --output-filename (=file name only). The ability to direct output to a specific absolute location has been lost.

@alexander-akait
Copy link
Member

@Rob--W you have configurations so you can setup this, it is breaking change and described

Rob--W added a commit to Rob--W/webextension-polyfill that referenced this pull request Oct 14, 2020
Currently the build/test fails with the following output:

```
Test webextension-polyfill bundled with webpack
===============================================
[webpack-cli] Unknown argument: --output
[webpack-cli] Unknown argument: /tmp/webpack-bundle.js
? Which flags do you want to use? …
✔ --entry: The entry point(s) of your application e.g. ./src/main.js
✔ --config: Provide path to a webpack configuration file e.g. ./webpack.config.js
✔ --color: Enable/Disable colors on console
✔ --merge: Merge two or more configurations using webpack-merge e.g. -c ./webpack.config.js -c ./webpack.test.config.js --merge
✔ --progress: Print compilation progress during build
✔ --help: Outputs list of supported flags
✔ --output-path: Output location of the file generated by webpack e.g. ./dist/
```

Regressed by:
webpack/webpack-cli#1855
webpack/webpack-cli@2f36b9d

`--output` has been renamed to `--output-path` (`-o` in both cases), but
stopped supporting file names.  To output to a specific absolute path,
`--output-path` and `--output-filename` must both be specified, with the
directory in the former and the filename in the latter.
@Rob--W
Copy link

Rob--W commented Oct 14, 2020

It seems that there is no way to pass an absolute file path via the command line any more, is there?
I fixed my use case by splitting the path and separately passing --output-path and --output-filename: https://github.com/mozilla/webextension-polyfill/pull/245/files

This limitation is strange though. Most tools that I can recall supporting a parameter to an output file accept an absolute path, whereas this tool now requires to separate flags. Is there any reason for this change?

@alexander-akait
Copy link
Member

alexander-akait commented Oct 14, 2020

@Rob--W because path and filename is different things, you can use placeholders in filename, so we don't know in --output path/to/[hash][ext] [hash][ext] is filename or placeholders, so we separate this

@Rob--W
Copy link

Rob--W commented Oct 14, 2020

-o /path/to/plain/file/without/webpack/placeholders.js is unambiguous. If the [hash] and [ext] are the primary reason for this breaking change, then I think that restoring support for --output [absolute path here] makes sense.

The meaning of the -o flag has also changed by this PR. It used to be a file path, now it's supposedly a directory path.

If you consciously accepted these breaking, backwards-incompatible changes, then feel free to ignore my comment.

Rob--W added a commit to Rob--W/webextension-polyfill that referenced this pull request Oct 14, 2020
Currently the build/test fails with the following output:

```
Test webextension-polyfill bundled with webpack
===============================================
[webpack-cli] Unknown argument: --output
[webpack-cli] Unknown argument: /tmp/webpack-bundle.js
? Which flags do you want to use? …
✔ --entry: The entry point(s) of your application e.g. ./src/main.js
✔ --config: Provide path to a webpack configuration file e.g. ./webpack.config.js
✔ --color: Enable/Disable colors on console
✔ --merge: Merge two or more configurations using webpack-merge e.g. -c ./webpack.config.js -c ./webpack.test.config.js --merge
✔ --progress: Print compilation progress during build
✔ --help: Outputs list of supported flags
✔ --output-path: Output location of the file generated by webpack e.g. ./dist/
```

Regressed by:
webpack/webpack-cli#1855
webpack/webpack-cli@2f36b9d

`--output` has been renamed to `--output-path` (`-o` in both cases), but
stopped supporting file names.  To output to a specific absolute path,
`--output-path` and `--output-filename` must both be specified, with the
directory in the former and the filename in the latter.
Rob--W added a commit to mozilla/webextension-polyfill that referenced this pull request Oct 15, 2020
Currently the build/test fails with the following output:

```
Test webextension-polyfill bundled with webpack
===============================================
[webpack-cli] Unknown argument: --output
[webpack-cli] Unknown argument: /tmp/webpack-bundle.js
? Which flags do you want to use? …
✔ --entry: The entry point(s) of your application e.g. ./src/main.js
✔ --config: Provide path to a webpack configuration file e.g. ./webpack.config.js
✔ --color: Enable/Disable colors on console
✔ --merge: Merge two or more configurations using webpack-merge e.g. -c ./webpack.config.js -c ./webpack.test.config.js --merge
✔ --progress: Print compilation progress during build
✔ --help: Outputs list of supported flags
✔ --output-path: Output location of the file generated by webpack e.g. ./dist/
```

Regressed by:
webpack/webpack-cli#1855
webpack/webpack-cli@2f36b9d

`--output` has been renamed to `--output-path` (`-o` in both cases), but
stopped supporting file names.  To output to a specific absolute path,
`--output-path` and `--output-filename` must both be specified, with the
directory in the former and the filename in the latter.
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.

Bug: Output flag should only set path
5 participants