kealjones-wk commented 4 years ago

Issue

An issue was discovered today that prevents socketcluster_client from being used with js/web based projects due to dart 2 now rejecting import 'dart:io'; being in a web project at all even when only being imported transitively and no code for it was executed, which was previously done in order to use the IoSocketPlatform type in socket.dart.

Fix

Isolate the dart:io and dart:html import containing files behind conditional imports.
Add SocketPlatform and other Base Types into separate interface class so they can be used for type checking.
Add initial assignment to _globalSocketPlatform through a conditional import RuntimeSocketPlatform constant in the respective file, this makes the platform be auto configured!
- Web users will no longer need to add globalSocketPlatform = httpSocketPlatform; as the conditional import does this for them.

QA

[ ] Flutter based projects continue to work as expected. (@pauldemarco any chance you could test this?)
[x] Web projects no longer have build time issues due to import. (confirmed by @kealjones-wk with an over_react project)

kealjones-wk commented 4 years ago

@pauldemarco This is ready for your review, dart decided to change things on us so the old method no longer works for web projects, i am unsure about vm/flutter projects, but this PR should remove the issue for everyone hopefully. If you have any questions/concerns, let me know :) Thanks!

MichaelMarner commented 4 years ago

I'm just testing this against Remote Devtools and have found issues trying to get the Socket to connect on Mobile.

I believe Remote Devtools is currently broken on mobile platforms

MichaelMarner commented 4 years ago

@kealjones-wk I am about to make a PR to your fork which fixes the platform selection between web and dart.io - looks like it has been broken for a while. If you merge my PR into your branch, we should be good

kealjones-wk commented 4 years ago

@MichaelMarner got your changes merged in and tested on web all is well on my end, sorry again if my previous pr unknowingly broke your repo MichaelMarner/dart-redux-remote-devtools, haha that was the opposite of what I wanted I was trying to make it usable for dart web projects with Workiva/over_react. We've got a react-redux wrapper in the works and your lib + this are essential to the dev flow :)

MichaelMarner commented 4 years ago

This should be fine to merge now - I have verified it still works in Flutter mobile, and @kealjones-wk has verified things work in web.

pauldemarco commented 4 years ago

LGTM

pauldemarco commented 4 years ago

New version 0.2.0 has been published to pubdev. https://pub.dev/packages/socketcluster_client

pauldemarco / socketcluster_client

Update SocketPlatform setup strategy #10

Issue

Fix

QA