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

Enable Dart 2.0 support #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Stargator
Copy link

@Stargator Stargator commented Aug 8, 2018

Since this project provides the web_server executable, I need to update it for Dart 2.0

Dependent on the following PRs in other repos:

@bwhite000 bwhite000 self-assigned this Aug 8, 2018
@bwhite000
Copy link
Owner

Hi @Stargator! Thank you so much for the work and contributions that you've done! I wasn't aware that my project had any current users until recently. I'll start up working on it again with ways that I have learned to write better code since the last commit.

I'll start reviewing your pull request after work this afternoon. Please know that I greatly appreciate your efforts and that any comments that I make aren't detracting from that appreciation at all :)

@bwhite000 bwhite000 added this to the WebServer v2.1 milestone Aug 8, 2018
@bwhite000 bwhite000 self-requested a review August 8, 2018 22:00
pubspec.yaml Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
example/virtual_directory.dart Show resolved Hide resolved
example/web_server_misc.dart Show resolved Hide resolved
lib/src/web_server/web_socket_server_request_handler.dart Outdated Show resolved Hide resolved
lib/src/web_server/web_socket_server_request_handler.dart Outdated Show resolved Hide resolved
bin/web_server.dart Show resolved Hide resolved

this._possibleFiles[urlData.path] = urlData.id;

this._functionStore[urlData.id].listen((final dynamic httpRequest) async {
Copy link
Owner

Choose a reason for hiding this comment

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

I'll make a note here, too, to change supporting code so that this can be typed again in the parameter field under Dart 2.0.

lib/web_server.dart Outdated Show resolved Hide resolved
@bwhite000
Copy link
Owner

@Stargator, I left some comments and praises in the GitHub review. Thanks again a million for the contributions!

@bwhite000
Copy link
Owner

I'll review the pull requests in the other dependent packages as soon as I next can 👍

@Stargator
Copy link
Author

@bwhite000 Ready for a re-review!

@Stargator
Copy link
Author

@bwhite000 I have added a change that is based on a PR for dart2_constant.

Those changes added io.HttpHeaders. Currently, dart2_constant doesn't support that. I'm working to fix that.

@Stargator
Copy link
Author

Nevermind, I removed those changes. I can incorporate it once dart2_constant is updated.

@Stargator
Copy link
Author

@bwhite000 I set it up so web_server and cache both use my fork of dart2_constant. The package has been archived by the Dart Team. I'm hoping to convince them to let me be the primary maintainer.

@Stargator
Copy link
Author

@bwhite000 Are you able to review the changes I made to your feedback?

Note that it is dependent on the other PRs in the original post being approved and merged for those dependent packages.

@Stargator
Copy link
Author

Stargator commented Jul 30, 2019

@bwhite000 Do note, the Dart SDK has made changes to what kind of dependencies new versions of a package can have on pub.dev. See dart-lang/sdk#36765.

From what I see, web-server and cache depend on my forked version of dart2_constant.

So either:

  1. You can merge in the changes before the next release of Dart that contains the new rule.
    • Then later you bump the version and drop Dart 1 support, thus drop the need for dart2_constant.
  2. I publish the forked version to pub.dev.

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

Successfully merging this pull request may close these issues.

2 participants