-
-
Notifications
You must be signed in to change notification settings - Fork 594
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
Output lib array opt #562
Output lib array opt #562
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the commit message for commitlint to pass. Use one of these.
Eg. cli: output lib array opt
or
tests(bin): add output options tests
I just left for vacation and don't have access to my machine. And to be frank, I find the extra hoop here to make a simple contribution to be cumbersome. Is there a simple way within GitHubs workflow that you could make these sorts of (to people outside of the project maintenance mindset, annoying) changes? |
Don't worry, we can rewrite the message of the commit while we do the merge onto master! |
bin/convert-argv.js
Outdated
@@ -441,7 +441,10 @@ module.exports = function(...args) { | |||
|
|||
ifArg("output-library", function(value) { | |||
ensureObject(options, "output"); | |||
options.output.library = value; | |||
if (!options.output.library) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value could be an empty array from yargs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried this, does not work. Default value is not set if a value is given via the CLI - it isn't an initial value. However I did replace this code w/ ensureArray
expect(stdout[7]).toMatch(/index\.js.*\{0\}/); | ||
expect(stderr).toHaveLength(0); | ||
|
||
const output = require("fs").readFileSync(require("path").join(__dirname, "../../../js/bin/output/output-library-many/bundle.js"), "utf-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import and destruct at top level instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,5 @@ | |||
./index.js | |||
-o ../../../js/bin/output/output-library-single/bundle.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this is needed, what's the syntax in the other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed. only the original output test used this flag, I just copied from there.
9ee38f8
to
9625be6
Compare
@hoten Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ev1stensberg Please review the new changes. |
Addressed comments, pushed changes + squashed to one commit. |
9625be6
to
0fb0daa
Compare
I also wanted a way to quickly run just my tests, so I introduced an env filter for binCases
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
expect(stdout[7]).toMatch(/index\.js.*\{0\}/); | ||
expect(stderr).toHaveLength(0); | ||
|
||
const output = fs.readFileSync(path.join(__dirname, "../../../js/bin/output/output-library-single/main.js"), "utf-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be in an own variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See prev comment 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had another look, left some comments. Looks smooth so far, just some small fixes and then it's ready! 💯
expect(stdout[7]).toMatch(/index\.js.*\{0\}/); | ||
expect(stderr).toHaveLength(0); | ||
|
||
const output = fs.readFileSync(path.join(__dirname, "../../../js/bin/output/output-library-many/main.js"), "utf-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you abstract this a lil better? Quite long line and slashes that could be trimmed down using path.resolve
expect(stderr).toHaveLength(0); | ||
|
||
const output = fs.readFileSync(path.join(__dirname, "../../../js/bin/output/output-library-many/main.js"), "utf-8"); | ||
expect(output).toContain("window.key1=window.key1||{},window.key1.key2=function"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe match this assertion using the array indice and regexes
expect(stdout[7]).toMatch(/index\.js.*\{0\}/); | ||
expect(stderr).toHaveLength(0); | ||
|
||
const output = fs.readFileSync(path.join(__dirname, "../../../js/bin/output/output-library-single/main.js"), "utf-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See prev comment 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small changes left! 💙
@@ -0,0 +1,14 @@ | |||
"use strict"; | |||
|
|||
const fs = require("fs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const {readFileSync} = require('fs')
const {join} = require('path')
const output = readFileSync(join(__dirname, __dirname, "..", "..", ".., "js", "bin", "output", output-library-many", "main.js"), "utf-8")
expect(stderr).toHaveLength(0); | ||
|
||
const output = fs.readFileSync(path.join(__dirname, "../../../js/bin/output/output-library-many/main.js"), "utf-8"); | ||
expect(output).toContain("window.key1=window.key1||{},window.key1.key2=function"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(stdout[myIndices]).toMatch(/window.key1=window.key/*\{0\}/);
expect(stdout[7]).toMatch(/index\.js.*\{0\}/); | ||
expect(stderr).toHaveLength(0); | ||
|
||
const output = fs.readFileSync(path.join(__dirname, "../../../js/bin/output/output-library-single/main.js"), "utf-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
se prev comment
expect(stderr).toHaveLength(0); | ||
|
||
const output = fs.readFileSync(path.join(__dirname, "../../../js/bin/output/output-library-single/main.js"), "utf-8"); | ||
expect(output).toContain("window.key1=function"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see prev comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's lgtm here, but I think @sokra needs to open up to for webpack internals to use Object.keys(library)
and to fetch objects in key-value pairs instead
Seems like tests aren't failing. @sokra is this valid? |
They ain't failing...? |
Did so locally on mine, but saw the CI afterwards. Was weird.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally again and it works. Thanks @hoten !! 💙
Fixes #557