Skip to content

add web compiler option to pubspec.yaml, change --compiler to --web-compiler #1624

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 27, 2017
Merged

add web compiler option to pubspec.yaml, change --compiler to --web-compiler #1624

merged 6 commits into from
May 27, 2017

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented May 25, 2017 •
edited
Loading

With the top level key in the pubspec being called js-compiler, it no longer makes sense for the command line arg to be just compiler (same argument holds about future compilers conflicting). Since js-compiler is really long I talked with @munificent and we landed on just --js.

Let me know if there are objections to changing the flag from --compiler to --js. If we could come up with a single character shorthand for --js-compiler that might be preferable, but I don't know of one that really makes sense.

cc @kevmoo @munificent @jwren @devoncarew

EDIT: See comment below about naming changes

@jakemac53 jakemac53 added the type-enhancement A request for a change that isn't a bug label May 25, 2017
@jakemac53 jakemac53 requested a review from nex3 May 25, 2017 17:18
@jakemac53
Copy link
Contributor Author

jakemac53 commented May 25, 2017 •
edited
Loading

Ok, after some offline discussion I will update this to change the arg to --web-compiler, and I am going to change the pubspec config to have a top level web key, with compiler as a key under that, so it will look like this:

web:
  compiler:
    debug: dartdevc
    release: dart2js

@jakemac53
Copy link
Contributor Author

Updated now to reflect the new configuration.

@jakemac53 jakemac53 changed the title add js_compiler key to pubspec.yaml, change --compiler to --js add web compiler option to pubspec.yaml, change --compiler to --web-compiler May 25, 2017
@munificent
Copy link
Member

👍 to the name change.

nex3
nex3 suggested changes May 26, 2017
View reviewed changes
lib/src/command/barback.dart Outdated
"Prefer using the --web-compiler arg as --[no]-dart2js is "
"deprecated.");
}
if (argResults['dart2js']) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this produce a warning as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lib/src/command/barback.dart Outdated
} else if (argResults.options.contains("compiler")) {
return Compiler.byName(argResults["compiler"]);
} else if (argResults.options.contains("web-compiler") &&
argResults.wasParsed("web-compiler")) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just argResults["web-compiler"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, there was a default value but I removed it as well.

lib/src/pubspec.dart Outdated
'"compiler" keys may only contain letters, '
'numbers, hyphens and underscores.',
key.span);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why this restriction? As far as I remember, there aren't any such restrictions on Barback mode names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok, I guess I was just assuming this, removed.

lib/src/pubspec.dart Outdated
/// the value is the name of the web compiler to use in that mode.
///
/// Valid compiler values are all of [Compiler.names].
Map<String, String> get webCompiler {
Copy link
Member

Choose a reason for hiding this comment

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

Why not Map<String, Compiler>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sure, I like that, updated.

test/pubspec_test.dart
test("can be empty", () {
var pubspec = new Pubspec.parse('web: {}', sources);
expect(pubspec.webCompiler, isEmpty);
});
Copy link
Member

Choose a reason for hiding this comment

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

Also can be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get a YamlMap in that case, but I added this to the "throws if not a map" test.

Copy link
Member

Choose a reason for hiding this comment

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

I believe in general we allow users to explicitly specify null for fields that are allowed to not exist, which means web: without anything after it is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done. I also added a test for a null compiler value

nex3
nex3 suggested changes May 27, 2017
View reviewed changes
test/pubspec_test.dart
test("can be empty", () {
var pubspec = new Pubspec.parse('web: {}', sources);
expect(pubspec.webCompiler, isEmpty);
});
Copy link
Member

Choose a reason for hiding this comment

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

I believe in general we allow users to explicitly specify null for fields that are allowed to not exist, which means web: without anything after it is valid.

lib/src/command/barback.dart
}
} else if (argResults.options.contains("web-compiler") &&
argResults.wasParsed("web-compiler")) {
} else if (argResults.options.contains("web-compiler")) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not quite sure why this isn't just argResults["web-compiler"] with a default value of false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't feel right to have a default value that isn't a valid compiler name to me. If you want I can do that though.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, I was thinking this was a boolean value for some reason.

@jakemac53 jakemac53 merged commit a878127 into master May 27, 2017
@jakemac53 jakemac53 deleted the pubspec-js-compiler-config branch May 27, 2017 00:54
@kevmoo
Copy link
Member

kevmoo commented May 27, 2017 via email

@jakemac53
Copy link
Contributor Author

cl out to merge into sdk here https://codereview.chromium.org/2908783003/ then I will send out the cherry pick request

@jakemac53
Copy link
Contributor Author

dart-lang/sdk#29741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@nex3 nex3 nex3 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@nex3 nex3

Labels
cla: yes type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakemac53 @munificent @kevmoo @nex3 @googlebot