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

Re-export HttpStatus from dart:html #34318

Closed
natebosch opened this issue Aug 30, 2018 · 13 comments

Comments

@natebosch
Copy link
Member

commented Aug 30, 2018

Since we allow imports to dart:io from code running with DDC/Dart2JS as long as none of the runtime behavior is used (as a hack to get around some other limitations) some code is using constants from dart:io like the ones in HttpStatus. There seem to be legitimate reasons to use these constants on the web - can we export them from both SDK libraries?

cc @vsmenon

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

Also CC @terrylucas

+1 though, seems like an easy fix.

@kevmoo

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

I'd rather NOT add more things to dart: libraries.

We should be moving the other way, IMHO

@natebosch

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

HttpHeaders also looks like it could be useful on the web.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

I'd rather NOT add more things to dart: libraries.

yeah I totally get where you're coming from (and agree). What's different about this one: we aren't really adding anything, just re-exporting something. Also integer constants like the ones on HttpStatus are "free" in dart2js (& should be in DDC too).

@grouma

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

Can we make a decision on this one so I can correctly communicate to our internal users? I am nearing the completion of the 'strict platforms' work which is directly impacted by this. Users are not able to import form dart:io and dart:html in the same library. Users should know if they need to create their own static utility class or wait until html supports these constants.

@vsmenon

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

Re-exporting in dart:html sgtm. We may want to eventually move this to a private (dart:_*) library and have dart:io and dart:html re-export that.

@rakudrama - any concerns for dart2js / tree-shaking? Right now, web folks are importing dart:io to get these.

@rakudrama

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

If you are suggesting

html_dart2js.dart:

export 'dart:io' show HttpStatus;

then sgtm.

But since dart2js will need to do some more work to load dart:io even when it is not used, and you will have to change the dart:html templates and regenerate the library, it is not much more effort to create dart:_http_constants now rather than later.

@vsmenon

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

@a-siva @zanderso - are you guys ok with moving some constants (e.g., HttpStatus ones) to a shared internal dart:_* file and re-exporting from dart:io and dart:html?

We're trying to snip web dependences on dart:io.

@zanderso

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

I think that should be fine, yes.

@vsmenon vsmenon added this to the Dart2.2 milestone Oct 4, 2018

@vsmenon

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@natebosch - can you enumerate the classes / constants we want to share and reassign to @terrylucas ?

@vsmenon vsmenon assigned natebosch and unassigned terrylucas Oct 4, 2018

@natebosch

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2018

HttpStatus for sure.

I was hoping for HttpHeaders because the constants are useful, however that class also has behavior that would likely be confusing to having in dart:http since all headers there are handled with Map<String, String> instead.

Let's go with just HttpStatus.

@terrylucas

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

dart:html changed with corresponding test CL pending.

@terrylucas

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Fixed with CL

@dart-bot dart-bot closed this in edca616 Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.