[google_maps_flutter] iOS TileOverlay stops loading after backgrounding app (3.13)

Is there an existing issue for this?

[X] I have searched the existing issues
[X] I have read the guide to filing a bug

Steps to reproduce

Run the google_maps_flutter example app on an iOS device (compile against a 3.13 flutter; this does not occur in 3.10)
Select the Tile Overlay example
Click Add Tile Overlay
Scroll the map, and observe how the custom tiles are loaded as expected
Background the app (can just trigger multitasking partially, for example)
Scroll the map again
Note that no new overlay tiles are downloaded, and the only tiles shown are the ones pulled down in steps 3-4

Expected results
As the user scrolls the map, custom TileOverlay tiles should continue to download and show whether or not the user has backgrounded the app yet.
Actual results
As soon as the user backgrounds the app, no more custom TileOverlay tiles will download.
This issue does not occur when compiling against Flutter 3.10. It does not seem to occur on Android.
Code sample
Reproduced using the example app in the packages repo: https://github.com/flutter/packages/tree/main/packages/google_maps_flutter/google_maps_flutter/example
Screenshots or Video
The following was captured from an iPhone 11 running iOS 17.0. I also reproduced it on an iPad Pro running iPadOS 16.7.

Video demonstration
https://github.com/flutter/flutter/assets/602697/81a6820d-0d18-4369-933e-00bf1bb0acd8

Logs
Logs
❯ fvm flutter run -d 00008030-000E10513A38802E
Launching lib/main.dart on Eli iPhone in debug mode...
Warning: Missing build name (CFBundleShortVersionString).
Warning: Missing build number (CFBundleVersion).
Action Required: You must set a build name and number in the pubspec.yaml file version field before submitting to the App Store.
Automatically signing iOS for device deployment using specified development team in Xcode project: 6L6G8SY9C6
Running pod install...                                             541ms
Running Xcode build...
Xcode build done.                                            6.1s
Installing and launching...
You may be prompted to give access to control Xcode. Flutter uses Xcode to run your app. If access is not allowed, you can change this through your Settings > Privacy & Security > Automation.
[VERBOSE-2:FlutterDarwinContextMetalImpeller.mm(37)] Using the Impeller rendering backend.
Installing and launching...                                        29.2s
Syncing files to device Eli iPhone...                               28ms

Flutter run key commands.
r Hot reload. 🔥🔥🔥
R Hot restart.
h List all available interactive commands.
d Detach (terminate "flutter run" but leave application running).
c Clear the screen
q Quit (terminate the application on the device).

A Dart VM Service on Eli iPhone is available at: http://<IP_ADDRESS>:51754/guQumPzfAk4=/
The Flutter DevTools debugger and profiler on Eli iPhone is available at: http://<IP_ADDRESS>:9102?uri=http://<IP_ADDRESS>:51754/guQumPzfAk4=/
[VERBOSE-2:shell.cc(1004)] The 'plugins.flutter.dev/google_maps_ios_0' channel sent a message from native to Flutter on a non-platform thread. Platform channel messages must be sent on the platform thread. Failure to do so may result in data loss or crashes, and must be fixed in the plugin or application code creating that channel.

Those ((null)) console messages begin once you start scrolling the map after backgrounding/foregrounding the app, and they keep generating after that point whenever you scroll the map again.

Flutter Doctor output
The output below is for 3.13.3, but I've reproduced it with 3.13.5 as well. The issue does not occur with 3.10.7.
Doctor output
❯ fvm flutter doctor -v
[✓] Flutter (Channel stable, 3.13.3, on macOS 14.0 23A339 darwin-arm64, locale en-US)
    • Flutter version 3.13.3 on channel stable at /Users/eligeller/fvm/versions/3.13.3
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision<PHONE_NUMBER> (2 weeks ago), 2023-09-06 14:32:31 -0700
    • Engine revision b8d35810e9
    • Dart version 3.1.1
    • DevTools version 2.25.0

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
    • Android SDK at /Users/eligeller/Library/Android/sdk
    • Platform android-34, build-tools 34.0.0
    • Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b829.9-10027231)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 15.0)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 15A240d
    • CocoaPods version 1.12.1

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2022.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b829.9-10027231)

[✓] VS Code (version 1.82.2)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.72.0

[✓] Connected device (5 available)
    • Eli’s ReportAll iPhone (mobile) • 00008110-000611A60CD8401E • ios            • iOS 17.0 21A329
    • Eli iPhone (mobile)             • 00008030-000E10513A38802E • ios            • iOS 17.0 21A329
    • Eli’s iPad Pro (mobile)         • 00008103-001A203E26C3001E • ios            • iOS 16.7 20H19
    • macOS (desktop)                 • macos                     • darwin-arm64   • macOS 14.0 23A339 darwin-arm64
    • Chrome (web)                    • chrome                    • web-javascript • Google Chrome 116.0.5845.187

[✓] Network resources
    • All expected network resources are available.

• No issues found!

Just an impact note... my team is OK to use the 3.10.7 release of Flutter as a workaround for now, but I really hope there's some fix or other workaround so that we can use the 3.13 releases, and reap the iOS performance gains there.

Hi, just checking in. Any more details on this one? Thanks!

No surprise here, but this isn't fixed in the newly stable Flutter 3.16.0. Our app is still back on 3.10.7 due to this iOS only issue.
Any idea what layer of the stack the issue is occurring in? Anything I can help test?
We're eager to move to a new Flutter, so if there's anything I can do to assist, Please ket me know! Thanks.

any news on the issue?

Any fix for this?

Anything I can help test?

Anyone is certainly welcome to help investigate. Bisecting to a specific revision on master where this started occurring would be one thing that could help narrow it down.

Thank you @stuartmorgan for the assistance. I ran bisected the master branch of Flutter down to this commit:
1f09a8662dad3bb1959b24e9124e05e2b9dbff1d is the first bad commit.
I see some references to Impeller here, including something about "wide gamut".

Sorry, it looks like I referenced the wrong repository in that link
Here is the correct link to the breaking commit
And here is the commit comparison where I noticed Impeller and wide gamut.

Good news… @martyfuhry remembered that a build flag was added in 3.13 relating to the wide gamut setting. He was able to find that by setting the FLTEnableWideGamut flag to false in the Info.plist file (or in the Custom Build settings in Xcode), the observed issue no longer occurred.
We verified that with the flag set to false, we are able to use the latest stable Flutter (3.16.9) without encountering this bug in iOS anymore. So this will hopefully serve as a good workaround for us, letting us use the latest and greatest Flutter releases, until the underlying bug is addressed.
Thanks,
Eli

Redirecting to the impeller team based on the above; cc @gaaclarke since it's related to wide gamut.

This example app in the "Tile Overlay" page never renders a map correctly
When disabling wide gamut the tile overlay appears to render correctly
With wide gamut enabled it never renders the overlay, instead issuing the following error logs when the "Add tile overlay" is pressed.

versions
flutter
commit 37fd173e03f33081ae75828272ca4d63c18aec75 (HEAD, origin/master, origin/main, origin/HEAD)
Author: David Martos<EMAIL_ADDRESS>Date:   Tue Feb 6 20:01:49 2024 +0100

packages
commit ef765f105069489dc8db93b568a00a043575f39f (HEAD -> main, upstream/main)
Author: dependabot[bot]<EMAIL_ADDRESS>Date:   Tue Feb 6 16:33:32 2024 +0000

log
[ERROR:flutter/shell/common/shell.cc(1038)] The 'plugins.flutter.dev/google_maps_ios_0' channel sent a message from native to Flutter on a non-platform thread. Platform channel messages must be sent on the platform thread. Failure to do so may result in data loss or crashes, and must be fixed in the plugin or application code creating that channel.
  I suspect we are serving textures to the google maps plugin and it is making assumptions about the pixel format of those textures, assuming r8g8b8a8 instead of 16bit float values for example.  It's likely the fix is that the plugin will have to support wide gamut textures, we don't want to make all of our plugins forced to render poorly.  Another solution could be to add an option for compatibility.  We'll see after I spend more time looking into it.

If you run the example with a debug engine it will crash on thread safety violations when "Add tile overlay" is pressed. So there is some shenanigans outside of wide gamut:
[FATAL:flutter/fml/memory/weak_ptr.h(109)] Check failed: (checker_.checker).IsCreationThreadCurrent().
Thread 7 Queue : com.google.Maps.TileStart-sdkLayer_0x280f57480 (concurrent)
#0	0x00000001e9b51578 in __pthread_kill ()
#1	0x000000020a9e4118 in pthread_kill ()
#2	0x00000001b20b0178 in abort ()
#3	0x000000010e8d0bc0 in fml::KillProcess() at /Users/aaclarke/dev/engine/src/flutter/fml/logging.cc:230
#4	0x000000010e8d0b18 in fml::LogMessage::~LogMessage() at /Users/aaclarke/dev/engine/src/flutter/fml/logging.cc:217
#5	0x000000010e8d0bdc in fml::LogMessage::~LogMessage() at /Users/aaclarke/dev/engine/src/flutter/fml/logging.cc:143
#6	0x000000010dc61a78 in fml::WeakPtr<flutter::PlatformView>::CheckThreadSafety() const at /Users/aaclarke/dev/engine/src/flutter/fml/memory/weak_ptr.h:109
#7	0x000000010dc547a8 in fml::WeakPtr<flutter::PlatformView>::operator->() const at /Users/aaclarke/dev/engine/src/flutter/fml/memory/weak_ptr.h:99
#8	0x000000010dc5b724 in -[FlutterEngine sendOnChannel:message:binaryReply:] at /Users/aaclarke/dev/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm:1255
#9	0x000000010f918ef4 in -[FlutterBinaryMessengerRelay sendOnChannel:message:binaryReply:] at /Users/aaclarke/dev/engine/src/flutter/shell/platform/darwin/common/framework/Source/FlutterBinaryMessengerRelay.mm:34
#10	0x000000010f91d218 in -[FlutterMethodChannel invokeMethod:arguments:result:] at /Users/aaclarke/dev/engine/src/flutter/shell/platform/darwin/common/framework/Source/FlutterChannels.mm:294
#11	0x000000010226298c in -[FLTTileProviderController requestTileForX:y:zoom:receiver:] at /Users/aaclarke/.pub-cache/hosted/pub.dev/google_maps_flutter_ios-2.4.1/ios/Classes/FLTGoogleMapTileOverlayController.m:125
#12	0x000000010227f078 in -[GMSCustomTileService sendRequest:] ()
#13	0x000000010227f55c in -[GMSCustomTileService internalRequestTile:target:selector:allowNetworkFetch:] ()
#14	0x0000000103804520 in _dispatch_call_block_and_release ()
#15	0x0000000103806038 in _dispatch_client_callout ()
#16	0x00000001038092f4 in _dispatch_continuation_pop ()
#17	0x0000000103808404 in _dispatch_async_redirect_invoke ()
#18	0x000000010381a43c in _dispatch_root_queue_drain ()
#19	0x000000010381ae34 in _dispatch_worker_thread2 ()
#20	0x000000020a9ddda0 in _pthread_wqthread ()

I've confirmed that the bug is that the google maps plugin doesn't support wide gamut images.  The following patch fixes the problem by transforming the images to non-wide gamut images before serving them to the google maps plugin:
--- a/packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapTileOverlayController.m
+++ b/packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapTileOverlayController.m
@@ -123,39 +123,49 @@
                    zoom:(NSUInteger)zoom
                receiver:(id<GMSTileReceiver>)receiver {
   dispatch_async(dispatch_get_main_queue(), ^{
-    [self.methodChannel invokeMethod:@"tileOverlay#getTile"
-        arguments:@{
-          @"tileOverlayId" : self.tileOverlayIdentifier,
-          @"x" : @(x),
-          @"y" : @(y),
-          @"zoom" : @(zoom)
-        }
-        result:^(id _Nullable result) {
-          UIImage *tileImage;
-          if ([result isKindOfClass:[NSDictionary class]]) {
-            FlutterStandardTypedData *typedData = (FlutterStandardTypedData *)result[@"data"];
-            if (typedData == nil) {
-              tileImage = kGMSTileLayerNoTile;
-            } else {
-              tileImage = [UIImage imageWithData:typedData.data];
-            }
-          } else {
-            if ([result isKindOfClass:[FlutterError class]]) {
-              FlutterError *error = (FlutterError *)result;
-              NSLog(@"Can't get tile: errorCode = %@, errorMessage = %@, details = %@",
-                    [error code], [error message], [error details]);
-            }
-            if ([result isKindOfClass:[FlutterMethodNotImplemented class]]) {
-              NSLog(@"Can't get tile: notImplemented");
-            }
-            tileImage = kGMSTileLayerNoTile;
-          }
-
-          [receiver receiveTileWithX:x y:y zoom:zoom image:tileImage];
-        }];
+    [self.methodChannel
+        invokeMethod:@"tileOverlay#getTile"
+           arguments:@{
+             @"tileOverlayId" : self.tileOverlayIdentifier,
+             @"x" : @(x),
+             @"y" : @(y),
+             @"zoom" : @(zoom)
+           }
+              result:^(id _Nullable result) {
+                UIImage *tileImage;
+                if ([result isKindOfClass:[NSDictionary class]]) {
+                  FlutterStandardTypedData *typedData = (FlutterStandardTypedData *)result[@"data"];
+                  if (typedData == nil) {
+                    tileImage = kGMSTileLayerNoTile;
+                  } else {
+                    tileImage = [UIImage imageWithData:typedData.data];
+                    CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
+                    CGContextRef context = CGBitmapContextCreate(nil, tileImage.size.width, tileImage.size.height, 8, 0, colorSpace, kCGImageAlphaPremultipliedLast);
+                    CGContextDrawImage(context, CGRectMake(0, 0, tileImage.size.width, tileImage.size.height), tileImage.CGImage);
+                    CGImageRef image = CGBitmapContextCreateImage(context);
+                    tileImage = [UIImage imageWithCGImage:image];
+                    CGImageRelease(image);
+                    CGContextRelease(context);
+                    CGColorSpaceRelease(colorSpace);
+                  }
+                } else {
+                  if ([result isKindOfClass:[FlutterError class]]) {
+                    FlutterError *error = (FlutterError *)result;
+                    NSLog(@"Can't get tile: errorCode = %@, errorMessage = %@, details = %@",
+                          [error code], [error message], [error details]);
+                  }
+                  if ([result isKindOfClass:[FlutterMethodNotImplemented class]]) {
+                    NSLog(@"Can't get tile: notImplemented");
+                  }
+                  tileImage = kGMSTileLayerNoTile;
+                }
+
+                [receiver receiveTileWithX:x y:y zoom:zoom image:tileImage];
+              }];
   });
 }

+
 @end

 @interface FLTTileOverlaysController ()

I'll let the ecosystem team decide if they want to incur that cost or if they want to work with the google maps team to support wide gamut images ( @stuartmorgan ).

I'll let the ecosystem team decide if they want to incur that cost or if they want to work with the google maps team to support wide gamut images ( @stuartmorgan ).

It's ultimately the iOS team's call, but I would vote for us doing the conversion—or if there's a way the plugin client could reasonably do the conversion in Dart, maybe just document that in the README—until the underlying SDK supports it.
We should make sure we test with each version of the Google Maps SDK that we support to see if it's already fixed. @gaaclarke Which ios* version of the example app were you ultimately testing this in, and what version of GoogleMaps do you have in your Podspec.lock for that example?

Which ios* version of the example app were you ultimately testing this in

I was using //packages/google_maps_flutter/google_maps_flutter/example

and what version of GoogleMaps do you have in your Podspec.lock for that example?

I was at ef765f105069489dc8db93b568a00a043575f39f, GoogleMaps says 6.2.1.

That's the oldest version of GoogleMaps we support, so we should re-test with google_maps_flutter_ios/example/ios14/ and see if the newest version still has the problem. If so we can follow up with an issue against the Google Maps iOS SDK. If not we can bisect the other ios* examples to see when it wide gamut support was added.

@gaaclarke is this something you are going to keep pushing on? Does it need another assignee? This seems like an issue we need to get fixed.

@gaaclarke is this something you are going to keep pushing on? Does it need another assignee? This seems like an issue we need to get fixed.

I've communicated the problem and handed it off to the iOS team ( @hellohuanlin ).  The plan is that they will follow up with the Google Maps team and decide the best course of action after that.

I checked on latest GoogleMaps 8.4.0 (iOS 14) and I the TileOverlays still do not load, and I see
2024-02-12 12:11:55.406149-0800 Runner[4058:2457053] ((null)) was false: Don't support little endian bitmaps
2024-02-12 12:11:55.406202-0800 Runner[4058:2457053] ((null)) was false: Unable to create GMSImageRenderTile due to bad graphics data

We should:

File an issue with GoogleMaps to support wide gamut in the future.
Land @gaaclarke's workaround or similar since we won't be able to support the latest GoogleMaps for awhile (due to the iOS 14 minimum), even if they fixed it tomorrow.
Write a test that would have caught this.
https://github.com/flutter/packages/blob/6828aaaf488b2882be3eec7f367eabdebfda4aea/packages/google_maps_flutter/google_maps_flutter_ios/example/ios12/integration_test/google_maps_test.dart#L898

@hellohuanlin can you work on 2 and 3?  I will try to repro with a non-Flutter app and file the issue.

File an issue with GoogleMaps to support wide gamut in the future.

b/325076777

Disabling Impeller on iOS for my app and my layers are showing up.

This issue should be fixed now. Please let me know if you run into any issues!


Prevent hibernation with background activities

How to prevent the cell phone from hibernating with background activities?

@duarte-evocorp are you on iOS or Android?

Android

@duarte-evocorp what do you mean by hibernation? You mean the screen going to the lock scren if the user is not using the device?

This, I want the application when it is open to prevent the screen from turning off. This is already happening, but when he puts the app in the background a few minutes later he hangs up.

I want the screen to remain on even in the background.

@duarte-evocorp sorry, but that is not possible currently on Android.

I understand, thank you.


Updating and adding of various flag and crest URL.

Currently, the flag and crest URLs of the various states point to the Wikipedia website entry. (Where the user can choose the download the SVG or PNG file).
This can be improved by adding the path to the image itself instead.
For example in the file state/v1/johor.json the
"flag_url" information is currenty
"https://en.wikipedia.org/wiki/Johor#/media/File:Flag_of_Johor.svg"
when it should be instead
"https://upload.wikimedia.org/wikipedia/commons/5/5a/Flag_of_Johor.svg"
This is the same as several other entries.
Besides that, several states are also missing this information including, Kedah, Kelantan and a few others.

I can working on this?

Sure. Assigned it to you. 👍


Dev

#389
#419
#442
#436

@bonnielu it looks like the fix for #442 may have broken the unit test for other language pages?

There are no backend changes in this PR, but we're doing a complete run to make sure nothing breaks:

temporarily disabled the cron job
checkout dev branch, pull changes
python3 setup.py install --user
module load openmpi/gnu
nohup python3 batch.py --dry-run > pr-449.log &

Checking the following:

[ ] Search

[ ] Search by accession number
[ ] Search by lineage
[ ] Search by country
[ ] Search by country (with date filter)

[ ] Tree filtering
[ ] Download Time-scaled tree (nwk)
[ ] Download Time-scaled tree (csv)
[ ] Download Beadplot (svg)
[ ] Download Beadplot (nwk)
[ ] Beadplot cutoff slider
[ ] Expanding Beadplot
[ ] Translated pages
[ ] Changing the colour of the tree based on divergence, collection date, etc.
[ ] Left/Right arrow key to move between beads
[ ] Tooltips
[ ] Countries/Samples/Mutations tabs are populating the correct information

I've been having trouble getting the --watch option to work with pm2 reload to reload the node servers when a change to the data files is detected. It works with pm2 restart but this would restart both instances of the node server. As a workaround, to ensure zero downtime, we can just schedule a cron job to reload the node servers (one after the other), at midnight every day.


Apparently valid git uri not recognized

I am modifying an existing plugin and working on a branch. When I try to reference the branch name in my .vimrc, I get an error message saying the plugin name is invalid:

However, when I test this uri in git, it says it's good:

What am I missing here?

I don't think this sort of thing is supported in vundle, sorry. I recommend using the pinned: 1 config option and updating manually if necessary. Hope this helps!

Well... the documentation clearly states that vundle: "...also allows the use of any URI git recognizes." and this is a valid git uri.


Calling local fun's in constructors before all fields are assigned

The following code will seem fairly natural to most folk investigating pony:
class Foo
  let _a : I64
  let _b : I64

  new create() =>
    _a = 1
    _b = foo() // Boom!

  fun foo() : I64 =>
    2

However, it results in a compile issue of the following form:
/wat/huh.pony:11:13: receiver capability is not a subtype of method capability
    _b = foo() // Boom!
            ^
/wat/huh.pony:11:10: receiver type: Foo tag
    _b = foo() // Boom!
         ^
/wat/huh.pony:14:3: target type: Foo box
  fun foo() : I64 =>
  ^
/wat/huh.pony:11:13: this would be possible if all the fields of 'this' were assigned to at this point
    _b = foo() // Boom!

One workaround is to move foo to a primitive helper:
primitive FooHelper
  fun foo() : I64 =>
    2

class Foo
  let _a : I64
  let _b : I64

  new create() =>
    _a = 1
    _b = FooHelper.foo()

The error is helpful, in that it gives a clue as to the workaround. But if the fun
depends on fields that have already been defined the natural expectation is that
this would be acceptable (the error is still valid if as yet undefined fields are utilised).
A side effect of moving the fun to a helper is that assigned fields now need to be
added to the helper's signature accordingly.
When porting code from other languages this issue may occur frequently enough to be
distracting.
Are there other workarounds that may be preferable for other cases?
What is considered idiomatic here and/or are there other gotchas to be avoided?

The docs say that "The purpose of a constructor is to set up the internal state of the object being created", which you could argue that to do so you should be able call other methods belonging to that class. But since it says "object being created" I assumed that a constructor in Pony is a lot closer to __new__ in Python (where you actually control the instance creation) than __init__ where you already have an instance available.
Disclaimer: I just started reading about pony and it looks very interesting, but I have no actual experience.

Very true. The constraint is necessary and sensible, but there's room for relaxing the constraint to allow already assigned fields to be leveraged to compute and assign subsequent fields. Sometimes a helper will be the right choice... I guess my (and by projection other folks investigating pony) hardwired polyglot brain finds the constraint unusual and distracting...
So, i'd be interested in learning about what related cases would surprise, and what the idiomatic resolutions are... ( a healthy dose of self-interest here, as I suspect I'll find out either way! )

Not sure about other resolutions, but as I understand it, the reason for the constraint is to ensure that it is statically impossible for any code to access a field which has not yet been initialized with a value.  The easiest way to do this is to put in place the constraints you've found.
Theoretically it would be possible to statically guarantee that any methods called from a constructor (and any methods they call by extension) do not read from any fields that are not yet initialized at the point of the call site.
The questions of whether the Pony team would find relaxing the constraint to be useful/desirable or the extra effort worthwhile are not for me to say.  I personally have mixed feelings about the desirableness - on the one hand, dealing with the constraint can feel distracting as you say - on the other hand, I can also see how the error message you would see after refactoring under a more liberal constraint (where you re-order method calls, or accidentally add a read reference to a method to violate the constraint) may be more confusing still.

As @jemc says there are 2 key issues with relaxing this constraint:

The amount of work required in the compiler to track which functions (and other transitively callable functions, including into other types) may access each field.
Whether the inconsistency would be confusing. Determining exactly which fields each function accesses is not possible (due to Halting Problem type arguments), so the compiler would have to just do its best. That leaves the programmer potentially in the situation where whether they can do something depends on whether the compiler is clever enough to spot it's OK. This is not very desirable and in general we prefer simple yes/no rules.

@jemc @andymcn Makes sense. I guess it's a case of convenience over correctness. If convenience cannot be provided with the guarantees offered by correctness then it's best to err on the side of correctness as trading away correctness is of course an inconvenience.

Under some circumstances it might be possible to statically determine that a a fun doesn't reference uninitialised fields, but in general that's undecidable.
In your particular example though, just do this:
class Foo
  let _a : I64
  let _b : I64

  new create() =>
    _a = 1
    _b = foo() // Allowed!

  fun tag foo() : I64 =>
    2

Here, since we can trivially prove that foo() doesn't read any uninitialised fields (because it can't read any fields), we can use it during construction.


XCODE Build results in 0x6f697463

Steps to Reproduce
When I try to archive my Build and prepare for TestFlight Upload the build warning is as follows:
**Building AOT snapshot in release mode (ios-release)...
Building App.framework for arm64...
Building App.framework for armv7...
Building AOT snapshot in release mode (ios-release)...             61,9s
Built to build/aot/.
warning: parsing line table prologue at offset 0x6f697463 found unsupported version 0x00
warning: line table parameters mismatch. Cannot emit.
note: while processing /Users/xxxxx/Documents/xxxxx/xxxxx/build/aot/armv7/snapshot_assembly.o
Project /Users/xxxxx/Documents/xxxxx/xxxxx built and packaged successfully.**
The Build seems the be successful but when I upload onto Testflight the following error occurs:
ITMS-90338: Non-public API usage - The app references non-public symbols in Frameworks/Flutter.framework/Flutter: _ptrace. If method names in your source code match the private Apple APIs listed above, altering your method names will help prevent this app from being flagged in future submissions.
What I have done: flutter clean and I remove the entire .framework project. I have updated flutter and reinstalled the pods, always the same error.
I cannot find anything that's related to _ptrace.
Logs

[✓] Flutter (Channel master, v1.9.8-pre.1, on Mac OS X 10.14.6 18G87, locale en-ZA)
    • Flutter version 1.9.8-pre.1 at /Users/martinspautz/flutter
    • Framework revision 9f4ab273f1 (4 days ago), 2019-08-28 20:02:08 -0700
    • Engine revision f52c0b9270
    • Dart version 2.5.0

[✓] Android toolchain - develop for Android devices (Android SDK version 28.0.3)
    • Android SDK at /Users/xxx/Library/Android/sdk
    • Android NDK location not configured (optional; useful for native profiling support)
    • Platform android-28, build-tools 28.0.3
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_152-release-1343-b01)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 10.3)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 10.3, Build version 10G8
    • CocoaPods version 1.7.5

[✓] Android Studio (version 3.4)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin version 38.2.1
    • Dart plugin version 183.6270
    • Java version OpenJDK Runtime Environment (build 1.8.0_152-release-1343-b01)

[✓] VS Code (version 1.37.1)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.3.0

[✓] VS Code (version 1.21.1)
    • VS Code at /Users/xxx/Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.3.0

[!] Connected device
    ! No devices available

! Doctor found issues in 1 category.

The only issue here is that my iPhone is not connected.

Duplicate of #37850

@stuartmorgan when you said this was a dup, were you saying that because of the e-mail response from that @southernsons received or were you referring to the warnings. or both?
warning: parsing line table prologue at offset 0x6f697463 found unsupported version 0x00
warning: line table parameters mismatch. Cannot emit.

Because I'm getting these warnings too even after I followed all the suggestions described in #37850.  I've been able to upload my app to TestFlght without any ITMS e-mails from apple, but  I'm concerned I have a ticking time-bomb somewhere in the app that is related to the warnings.
Any additional insight would be appreciated.

were you saying that because of the e-mail response from that @southernsons received or were you referring to the warnings. or both?

The former; despite the title of the issue, the primary concern seemed to be the rejection due to _ptrace. It's likely that the warning is unrelated given that you see the warning but not the rejection (it's common for people to only notice warnings when there's some later error, and then assume that the warning is related).
I would suggest filing a new bug about the warning; I would re-open this one, but I think the inclusion of the _ptrace rejection would confuse things.

Thanks @stuartmorgan for the insight.  I downgraded flutter to the 1.7.8 hotfix4 release this morning and the warnings disappeared. If I see the warnings again when I upgrade back to the Flutter 1.9.x release (after I get a few action items off my plate) I will post a bug.

I downgraded to 1.7.8 and then upgraded again to v1.9.1+hotfix.4. Still the same error, which is critical.
I currently can build apk --release and ios --debug
but can't build ios --release

Yes, critical. Can't upload to iOS as of this.

Again, this bug has been closed as a duplicate. You want to follow the bug referenced above, not this one.


Is there a way to merge Sevilleta and North Arm?

The two stations are currently handled as separate branches - main for Sevilleta and lwana for North Arm.  Is there a way to merge them?  Some things to consider:

They have different numbers of stands.  This changes:

Rings sizes
How the spectra figures are divided up

Different disk sizes for storage management
Different targets for uploading images/spectra/movies to

Could these differences be handled by a configuration file?

They are much closer now.  Key differences:

Station setup (lsl.common.ndp vs. lsl.common.adp, etc.)
minval/maxval setup in _plot_spectra()
phase center control
imager setup (50 m station vs. 100 m station)
conjugation on the data for imaging (no vs yes)
how to deal with which frequencies to save


How to enable sourcemap?

With the '--debug', browserify only generate map for riot.js. Can we have source map also for .tag files?

I figured out, by using minifyify:
browserify -t riotify -d src/app.js -p [minifyify --compressPath . --map app.js.map --output dist/app.js.map] -o dist/app.js

See also https://github.com/muut/riotjs/issues/219

Really don't follow what happens on that minifyify command. Any chance to add a Sourcemap- section to the compiler doc?
Thanks!

@tipiirai: Not until riot support source maps. See muut/riotjs#219


Internal logging improvements

Found myself needing to do some of this (the OpenTelemetry part, specifically, for the SQLAlchemy instrumentation) and wishing (earlier on, for the heartbeats and probes) that I did not have to initialise a logger instance in order to log something, and that a genuine trace-level logging option was available.
[skip changeset] because none of this really changes anything for customers (who should not be using "trace")
Move internal logger to separate module
The logger was kind of "bolted on" to the client class -- when other
parts of the code wanted to use it, they needed to fetch the logger
with the same name separately and deal with it as a variable.
Move this to a separate internal_logger module which is initialised
on-demand from the existing initialised (not necessarily started!)
global client.
Implement trace logging level
Make it possible to meaningfully log "trace" logging level messages
(meaning they're logged on trace log level, and not logged on debug
log level) like in our other integrations.
Log OpenTelemetry spans on trace
When logging level is set to "trace", log all OpenTelemetry spans to
the console.
This is similar to what the Node.js integration does with the
OpenTelemetry diagnostics, which does not seem to have a direct
equivalent in Python's OpenTelemetry implementation.
Log instrumentation only when successful
Do not log "Instrumenting ..." when attempting to start an
instrumentation (the required dependency for which might not actually
be installed)
Instead, log "Instrumented ..." when an instrumentation is correctly
started.

@tombruijn Thanks, that makes sense. Restoring those to "debug".
Perhaps my thinking about "trace" and "debug" is wrong? The way I think about it is that "debug" is something that you could enable in production for a not-too-short period of time to figure out an issue. As such I am trying to avoid "spam" of the "Did thing successfully!" variety that would clog the logs -- especially with probes, where it would happen once per minute.

My thoughts about it are, that things that are useful for us and users to see in the logs to debug issues are useful as debug logs. That can include "did the thing", if we want to know if the thing was done, like adding instrumentations if we don't see any spans/metrics from them, or if an event (heartbeat, breadcrumb, probe) was sent/triggered or not. Sometimes, not seeing an error about a thing not working, is not good enough if we can't tell if the thing actually was run/started or not.
The trace messages log internal messages, things like received OpenTelemetry export requests, requests made to the Push API and public endpoint. That's the debug logging we want to see when we're debugging something deep in the integration or agent, without having to 1) add a new log message to the agent, 2) build a new agent, 3) add that custom agent build to the app, every time.
I also wrote down some short notes about this when I changed some log levels in the agent some time back. https://github.com/appsignal/appsignal-agent/pull/1072
There I promoted a lot of span/transaction API trace logs to the debug level, so it wouldn't require us to always enable to "trace" level (making the debug level useless), including all the internal noise users and us in support don't need.
In this example for probes, I think it's useful to log which probe is run as a debug message. That's because we don't log the collection of probes when the probe system starts. If we would log which probes are active at the start, we could log those as a debug message, and (if needed) log which probes run every minute as a trace mesasge. (Keep that in mind that if a probe gets added after the probe system is started, I would want to see that new probe also being logged as a debug message when it gets added.)

@tombruijn This is very useful, thank you! I'll maybe see about adding the probes logging you describe as well. (Maybe not, it can be done later)

I'll maybe see about adding the probes logging you describe as well. (Maybe not, it can be done later)

I wouldn't bother with it now. Let's do that in a separate PR?


[x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Pull Request Type

[x] :bug: Bug fix (Non-breaking Change: Fixes an issue)
[ ] :hammer_and_wrench: Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
[ ] :nail_care: New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

n/a - noticed in release
Risk
lower than before - but deals with account selection in swapper, so still needs a good test

Testing
switch accounts in the swapper, do a handful of trades. previously it was only selecting the highest balance account, now it should respect the selected account.

Engineering
see that we're using a real handler.

Operations
see above.

Screenshots (if applicable)
no visual changes in this PR - see #3772 for swapper UI changes

I did a swap from account 1 and confirmed is used the correct account for the trade.
Whilst testing I did find two more, similar bugs:

"Max" does not work for UTXOs, and always uses the balance of the highest account (or the first, I'm not sure - they are the same for me)
The (available) "Balance" does not update when an account selection is made

I'll approve this as it's strictly better than what's currently merged, but there are still bugs.
CC @shapeshift/operations & @reallybeard.

hmmm

i'm not able to reproduce
is currently present in prod too, but agree this could be improved, and i thought it was a bug too.


Added NAT-PMP, PCP, UPnP probing functionality

Probes the router for these port control protocols on the "Logs & Network Analysis" page.
Solves #1604.

Review status: 0 of 4 files reviewed, 10 unresolved discussions, all commit checks successful.

src/chrome/app/manifest.json, line 18 [r1] (raw file):
Adding new permissions, especially broad ones like this, is not very appealing.  I think you should be able to avoid the need to add this permission by using freedom['core.xhr']instead of XMLHttpRequest.

src/generic_core/diagnose-nat.ts, line 862 [r1] (raw file):
There's no need to wrap this in a function.  You can just compute the promise, and then drop that into Promise.race() at the end.  Same for the other functions.

src/generic_core/diagnose-nat.ts, line 867 [r1] (raw file):
Our style guide generally requires that ordinary variables start with lower case, so this would be pmpResponse.  (We make an exception for F and R.)

src/generic_core/diagnose-nat.ts, line 875 [r1] (raw file):
This should be a reject, not a resolve.  Same for the other functions.

src/generic_core/diagnose-nat.ts, line 935 [r1] (raw file):
Float?

src/generic_core/diagnose-nat.ts, line 939 [r1] (raw file):
I think this is a good place to use ipaddr.js

src/generic_core/diagnose-nat.ts, line 1027 [r1] (raw file):
"get" is a bit ambiguous.  How about "fetch"?

src/generic_core/diagnose-nat.ts, line 1135 [r1] (raw file):
I think this is overkill.  I would suggest just splitting on whitespace and pulling out the token at the appropriate index, then using ipaddr.js to determine if it's IPv4.

third_party/typings/generic/url.d.ts, line 22 [r1] (raw file):
Missing newline.

Comments from the review on Reviewable.io

lgtm, awesome work! I'm also just a little worried by the broad http permission in the manifest -- if we can get around that, that'd be ideal.

Review status: 3 of 4 files reviewed, 10 unresolved discussions, all commit checks successful.
Reviewed files:

src/chrome/app/manifest.json @ r1
src/generic_core/uproxy_core.ts @ r1
third_party/typings/generic/url.d.ts @ r1

Comments from the review on Reviewable.io

agree that the http://*/ permission is a bit scary.  Could make that go away by making the XHR through sockets. there are libraries to help with that exactly to avoid this sort of permission problem: https://github.com/ahmadnassri/chrome.sockets.tcp.xhr

Actually ... maybe it doesn't matter?  We should check whether it alters the prompt to the user.  Arguably, it should not, because the app already requests full TCP permission, so "http://*/" is not actually any additional privilege.

The permissions prompt with http://*/ is the same:
 - Communicate with cooperating websites
 - Exchange data with any device on the local network or internet

Review status: 3 of 4 files reviewed, 10 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

What about Firefox?

Review status: 3 of 4 files reviewed, 10 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

I only changed manifest.json, which only applies to Chrome. The new probing functionality works in Firefox without any extra permissions.

Review status: 3 of 4 files reviewed, 10 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

Review status: 1 of 4 files reviewed, 10 unresolved discussions, some commit checks pending.

src/chrome/app/manifest.json, line 18 [r1] (raw file):
Didn't change since this doesn't alter the user permission prompts in Chrome or Firefox. (See discussion above)

src/generic_core/diagnose-nat.ts, line 861 [r1] (raw file):
Done. probePMPSupport() and probePCPSupport() now resolve to booleans, while probeUPnPSupport resolves only to a true boolean (if everything goes smoothly) or rejects with an Error containing a detailed message. Booleans and Errors are converted into status strings in the wrapper functions in uproxy_core.ts.

src/generic_core/diagnose-nat.ts, line 862 [r1] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 867 [r1] (raw file):
Should be fixed here and in other variables.

src/generic_core/diagnose-nat.ts, line 875 [r1] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 935 [r1] (raw file):
It was slightly more compact than two setInt32()'s (there's no setInt64()). I changed it to the former for clarity, though.

src/generic_core/diagnose-nat.ts, line 939 [r1] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 1027 [r1] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 1135 [r1] (raw file):
Done.

third_party/typings/generic/url.d.ts, line 22 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Review status: 1 of 4 files reviewed, 11 unresolved discussions, all commit checks successful.

src/generic_core/diagnose-nat.ts, line 909 [r2] (raw file):
It looks to me like you are leaking this socket on every test.  Unfortunately, Freedom objects aren't garbage-collected, so you need to explicitly close them.  In fact, to be really clean, you should probably do
socket.close().then(() => {
  freedom['core.udpsocket'].close(socket);
});

The first line closes the OS-level socket, and the second line discards the Freedom object representing that socket.

src/generic_core/diagnose-nat.ts, line 917 [r2] (raw file):
Why not bind to privateIP?

src/generic_core/diagnose-nat.ts, line 1033 [r2] (raw file):
We try to always use === for consistency.

src/generic_core/diagnose-nat.ts, line 1078 [r2] (raw file):
Two spaces before an inline comment

src/generic_core/diagnose-nat.ts, line 1104 [r2] (raw file):
Our style guide excludes inline if { }

src/generic_core/diagnose-nat.ts, line 1120 [r2] (raw file):
80 column limit?

src/generic_core/diagnose-nat.ts, line 1123 [r2] (raw file):
No space between ? and :.

src/generic_core/diagnose-nat.ts, line 1127 [r1] (raw file):
Still overkill :).  Just hardcode the value of hostIndex and check if what's there is "host".

src/generic_core/diagnose-nat.ts, line 1138 [r2] (raw file):
4 space indent

src/generic_core/diagnose-nat.ts, line 1156 [r2] (raw file):
No space before : in arguments

src/generic_core/uproxy_core.ts, line 374 [r2] (raw file):
Rather than copy-paste this code between probePMP and probePCP, I think it would be better to have a single function that takes probePCPSupport or probePMPSupport as an argument.

Comments from the review on Reviewable.io

Review status: 1 of 4 files reviewed, 11 unresolved discussions, some commit checks pending.

src/generic_core/diagnose-nat.ts, line 909 [r2] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 917 [r2] (raw file):
Done. Changed all the probing functions to bind to privateIP for consistency.

src/generic_core/diagnose-nat.ts, line 1033 [r2] (raw file):
Done. (Changed everywhere)

src/generic_core/diagnose-nat.ts, line 1078 [r2] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 1104 [r2] (raw file):
Didn't change. (Style guide doesn't actually exclude it)

src/generic_core/diagnose-nat.ts, line 1120 [r2] (raw file):
Done. Fixed a few other places as well.

src/generic_core/diagnose-nat.ts, line 1123 [r2] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 1127 [r1] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 1138 [r2] (raw file):
Done.

src/generic_core/diagnose-nat.ts, line 1156 [r2] (raw file):
Done.

src/generic_core/uproxy_core.ts, line 374 [r2] (raw file):
Done.

Comments from the review on Reviewable.io

Review status: 1 of 4 files reviewed, 5 unresolved discussions, some commit checks failed.

src/generic_core/diagnose-nat.ts, line 909 [r2] (raw file):
What if there's no reply, so onData never fires?

src/generic_core/diagnose-nat.ts, line 1142 [r2] (raw file):
That's a 2-space indent.

src/generic_core/uproxy_core.ts, line 396 [r3] (raw file):
Nit: Generally, we don't break lines in a case like this.  (then would go on the preceding line.)

src/generic_core/uproxy_core.ts, line 406 [r3] (raw file):
Printing the same error message three times seems unhelpful.
I think this would be a lot clearer if the success/failure handling were independent (before the Promise.all).

Comments from the review on Reviewable.io

Review status: 1 of 4 files reviewed, 5 unresolved discussions, all commit checks successful.

src/generic_core/diagnose-nat.ts, line 909 [r2] (raw file):
Ah okay, I added a callback to destroy the socket when there's no reply/on timeout, which is wrapped in the new function closeSocket().

src/generic_core/diagnose-nat.ts, line 1142 [r2] (raw file):
Done.

src/generic_core/uproxy_core.ts, line 396 [r3] (raw file):
Done.

src/generic_core/uproxy_core.ts, line 406 [r3] (raw file):
The error handling is already managed independently in the wrapper functions probePMCP() and probeUPnP(); those should never reject and always return a status string. The .catch() here only applies to the error when getInternalIP() rejects.
However, I made the logic more sequential and compacted the three identical error messages into one.

Comments from the review on Reviewable.io

Review status: 1 of 4 files reviewed, 3 unresolved discussions, all commit checks successful.

src/generic_core/uproxy_core.ts, line 368 [r4] (raw file):
Nit: Don't put a line break here.

src/generic_core/uproxy_core.ts, line 383 [r4] (raw file):
No line break here.

src/generic_core/uproxy_core.ts, line 408 [r4] (raw file):
I think it would be better to move getNatType before getInternalIP, so you don't have to call it twice.

Comments from the review on Reviewable.io

Review status: 1 of 4 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

src/generic_core/uproxy_core.ts, line 368 [r4] (raw file):
Done.

src/generic_core/uproxy_core.ts, line 383 [r4] (raw file):
Done.

src/generic_core/uproxy_core.ts, line 408 [r4] (raw file):
Done.

Comments from the review on Reviewable.io

:+1:

Comments from the review on Reviewable.io


PowerToys Run:Translate Feature

Description of the new feature / enhancement
input a word then translate the answer, something like this:
abandon --> 抛弃，舍弃
Scenario when this would be used?
my english is poor, so sometime I have to open a  browser to translate a word when I don't know some word.
PowerToys is very convenient, so i am thinking if it has a translate feature will be great!
or create a syntactic sugar that link to translate website like "google translate", thanks!
Supporting information
No response

Thanks for your message. This is a Duplicate of an other issue. Additional comments may be added there. /dup #9095
I found a small script on Wiktionary that can be saved as a bookmark. If there is text selected - and you click the bookmark - it immediately searches for the word(s). If no text selected yet, it opens a small pop-up window to type in.
javascript:(function()
 {function se(d) {
    return d.selection ?%20d.selection.createRange().text%20:%20d.getSelection()}%20s%20=%20se(document);%20for%20(i=0;%20i%3Cframes.length%20&&%20!s;%20i++)%20s%20=%20se(frames[i].document);%20if%20(!s%20||%20s==%27%27)%20s%20=%20prompt(%27Enter%20search%20terms%20for%20Wiktionary%27,%27%27);%20open(%27https://en.wiktionary.org%27%20+%20(s%20?%20%27/w/index.php?title=Special:Search&search=%27%20+%20encodeURIComponent(s)%20:%20%27%27)).focus();
 })();


[WIP] Update coverage

I think I already merged this, can you please check?
https://github.com/ripple/rippled/commit/76b761d8e201c0a71de6112b0f1754ac15cdd1d7

Yes, you merged @nbougalis version with his gdb fixes


Release 2.0.0

Codecov Report

Merging #179 (a9d9b30) into master (ea83d9d) will increase coverage by 4.39%.
The diff coverage is 78.31%.

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   82.15%   86.54%   +4.39%
==========================================
  Files          15       16       +1
  Lines         566      617      +51
==========================================
+ Hits          465      534      +69
+ Misses        101       83      -18

Flag
Coverage Δ

unittests
86.54% <78.31%> (+4.39%)
:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

...UIPager/Helpers/OnAnimationCompletedModifier.swift
42.85% <42.85%> (ø)

Sources/SwiftUIPager/PagerContent.swift
55.38% <54.54%> (+12.64%)
:arrow_up:

Sources/SwiftUIPager/Page.swift
100.00% <100.00%> (ø)

Sources/SwiftUIPager/Pager+Buildable.swift
100.00% <100.00%> (ø)

Sources/SwiftUIPager/Pager.swift
93.10% <100.00%> (+4.21%)
:arrow_up:

Sources/SwiftUIPager/PagerContent+Buildable.swift
100.00% <100.00%> (ø)

Sources/SwiftUIPager/PagerContent+Helper.swift
97.56% <100.00%> (+0.19%)
:arrow_up:

... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea83d9d...a9d9b30. Read the comment docs.

Codecov Report

Merging #179 (a9d9b30) into master (ea83d9d) will increase coverage by 4.39%.
The diff coverage is 78.31%.

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   82.15%   86.54%   +4.39%
==========================================
  Files          15       16       +1
  Lines         566      617      +51
==========================================
+ Hits          465      534      +69
+ Misses        101       83      -18

Flag
Coverage Δ

unittests
86.54% <78.31%> (+4.39%)
:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

...UIPager/Helpers/OnAnimationCompletedModifier.swift
42.85% <42.85%> (ø)

Sources/SwiftUIPager/PagerContent.swift
55.38% <54.54%> (+12.64%)
:arrow_up:

Sources/SwiftUIPager/Page.swift
100.00% <100.00%> (ø)

Sources/SwiftUIPager/Pager+Buildable.swift
100.00% <100.00%> (ø)

Sources/SwiftUIPager/Pager.swift
93.10% <100.00%> (+4.21%)
:arrow_up:

Sources/SwiftUIPager/PagerContent+Buildable.swift
100.00% <100.00%> (ø)

Sources/SwiftUIPager/PagerContent+Helper.swift
97.56% <100.00%> (+0.19%)
:arrow_up:

... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea83d9d...a9d9b30. Read the comment docs.


⚡ Prevent stages beyond parsing when formatting

Resolves #178

I apologize if this is an overly simple solution, but it seemed to me that, if formatting the code is the user's goal, that no other phases should be performed (except maybe the analysis phase :thinking: ). Please let me know if the more changes should be made :smiley:

Cool ✌


Make NpmScriptRunner public

This class is very useful for starting other npm based projects and it is a shame that currently I need to duplicate it in my code.

Thanks for the suggestion. It's not something we specifically have planned to do at the moment (because every bit of public API we expose is a maintenance contract forever), but if others continue to show interest in this we could review that.

Thanks for the info, I'll extract it manually then.


Is this library's returned promise chainable/compatible with mocha's promise support?

Here are two tests I created, the first passes when it should not and the second correctly exits with an error. Is this a bug or am I under the wrong impression?

import agent from 'superagent';

describe('some tests', function() {

    describe('GET http://nowhere', function() {

        //Does not work as expected
        it('should fail', function() {
            return agent.get('http://nowhere').then(function() {
                       console.warn('this should never be called!');
             });
        });

       //works as expected
        it('should also fail ', function(done) {
            agent.get('http://nowhere').end(done);
            });
        });
    });

superagent: v3.1.0
mocha: v3.2.0
node: v4.0.0

It is compatible if you don't use .end(). Use of .end() breaks Promises.


How to handle wrong metadata

While running the example in 04_cardio_multiwell, from the fractal repo, the file /data/active/fractal/tests/20220920_10well_5x5/20200812-CardiomyocyteDifferentiation14-Cycle1.zarr/F/09/0/0/.zarray reads
{
    "chunks": [
        1,
        1,
        2160,
        2560
    ],
    "compressor": {
        "blocksize": 0,
        "clevel": 5,
        "cname": "lz4",
        "id": "blosc",
        "shuffle": 1
    },
    "dimension_separator": "/",
    "dtype": "<u2",
    "fill_value": 0,
    "filters": null,
    "order": "C",
    "shape": [
        3,
        19,
        10800,
        12799
    ],
    "zarr_format": 2
}

Note the 12799 dimension along the X direction.

We should at least include a check like
if shape_x % img_size_x > 0:
    raise Exception(...)

Is this related to the precision of the pixel sizes? Or any other idea where the off-by-one error is coming from?

For the record, this the .zarray for a simpler example (a single well with 9x8 FOVs, in /data/active/fractal/3D/PelkmansLab/CardiacMultiplexing/Cycle1_9x8_singleWell), which may be simpler to debug:
{
    "chunks": [
        1,
        1,
        2160,
        2560
    ],
    "compressor": {
        "blocksize": 0,
        "clevel": 5,
        "cname": "lz4",
        "id": "blosc",
        "shuffle": 1
    },
    "dimension_separator": "/",
    "dtype": "<u2",
    "fill_value": 0,
    "filters": null,
    "order": "C",
    "shape": [
        3,
        19,
        19439,
        20479
    ],
    "zarr_format": 2

The get_ROIs_bounding_box function was returning an array of floats for each shape element, instead of a single int. I don't know yet whether this explains the wrong shape, but for sure it had to be fixed.

It looks like the error is in the metadata file.
An image should have a X size of 416 micrometer (corresponding to 2560 pixels, with 0.1625 micrometer/pixel).
If I look at data/active/fractal/3D/PelkmansLab/CardiacMultiplexing/Cycle1_9x8_singleWell/MeasurementData.mlf, I see that the min/max values of X are:
X_min=-1448.3   # e.g. in 20200812-CardiomyocyteDifferentiation14-Cycle1_B03_T0002F009L01A01Z01C01.tif
X_max=1463.6    # e.g. in 20200812-CardiomyocyteDifferentiation14-Cycle1_B03_T0002F008L01A01Z01C01.tif

The difference between these two values is 2911.9 micrometers (instead of the expected 7*416=2912 micrometers), which is not an integer multiple of 416.
@jluethi @gusqgm, can you let us know how this works and how to proceed?
It's not clear whether this kind of error is a surprising behavior or something that we should support.

Thanks for tracing it there, I'll have a look into this. It's a bit surprising to me, but maybe those positions get rounded and we'll need to find a way to work with that precision. I'll look into it a bit more to get an idea. If there really is rounding of those positions, we'd have to check if there are potential off-by-1 errors also in the middle of a well, leading to a column of zeros or a column that gets overwritten by 2 FOVs due to that rounding.

Big picture: We'll need to actually handle that. It's not that the metadata is wrong, just that the precision is just slightly too low for how we're trying to use it here. We will need to find a workaround for that though.
Some first thoughts into this, but not finished yet:
The precision of positional metadata of the yokogawa is limited to 0.1 um. That is somewhat reasonable given the precision of the actual stage.
Pixel sizes in our test set are 0.1625 um. Other typical pixel sizes are 0.325 and 0.108333333. Thus, the precision of how stage positions are saved is unfortunately very close to the step size of our pixels, making an off-by-one error something we need to be careful about.
The lack of precision is not due to our rounding. e.g. in the case where the calculation says it's a distance of 2911.9 micrometers. With a pixel size of 0.1625, that would be 17’919.384 pixels. But we'd actually expect 17920 pixels. Thus, even rounding would get us to an off-by-one error in these calculations vs. the underlying assumed positions.
A few things worth looking into next:

If we just do those calculations for each FOV separately & round, would we be save then (against both + & - 1 errors)?
Do we have a way to calculate whether we would have ROI overlap and handle the case of an (unexpected) single-pixel overlap? Do the individual ROIs actually have a single pixel overlap or would we have an off-by-one error when trying to access the array somewhere?

This is trickier than I expected. The same error would pop up if we just go FOV-by-FOV.
Here are the FOV x_coordinate positions for the first row in a 9x8 well (23 well dataset, B03)

And here are the distances between the all FOVs based on the x_micrometer positions, as well as the corresponding pixel shifts when divided by the pixel size

One interesting observation here: The rounding error always occurs in the same place. Independent of the well or the row one looks at, it's always FOV 3 that has a distance of 415.9um, when it should really have a distance of 416 um.
I think this shows that it's really a rounding happening in the way Yokogawa saves this metadata, robustly rounding their stage position metadata.

What do we do with this?
On one level, we want to parse the real metadata and it's a tricky discussion on whether we should change any of the actual metadata when parsing data (preferably, we'd just save the actual metadata the microscope gave us).
Unfortunately, in this case, the microscope metadata is not accurate enough for what we need, making consistent tiny rounding errors that can shift things by a single pixel.
I think we could detect this after having parsed the table and correct the table in this simplest case (i.e. check that all FOVs don't have a single pixel overlap. When we detect single-pixel overlap, shift all FOVs to the left & bottom of that position by the required distance (e.g. by ~0.1um). But I'm not sure how well this generalizes. Here are a few of the concerns:

Changing metadata is not really what we want to be doing
Search-first data will not come in a clear grid pattern, thus the patterns wouldn't be as regular (though a single pixel overlap due to the lack of stage position precision could still occur and we probably would also want to shift that).
At some point, we'll tackle Yokogawa data with overlap (https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/10). But thinking about this now, my suggested approach there would also be to calculate the overlap, shift the FOVs to avoid the overlap, then save the data.

Having reflected a bit on these concerns, maybe everything comes together as an addition to the metadata parsing.
Maybe we have an unintentional case of "imaging with overlap", but we can actually implement a general solution that would also work for that scenario.
The rough idea:
We parse the metadata as before. But then we run another calculation on the data that will do the following: Check if any FOVs overlap, shift by however much is necessary to make the fields not overlap anymore. Save this new position as a separate column in the metadata. Any processing could then use that information (e.g. to store the data according to the new positions). Only potential stitching approaches (modality 2 here: https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/11) would require the original metadata. But given that it's still technically there, nobody can complain about not having the original metadata.

tl;dr: I think we can tackle this as an additional processing during the metdata parsing
And tackling this, if done correctly, will be useful for handling images with overlap


Modular API — separate history listening, route matching, and component rendering

Don't merge; using this PR for discussion
Follow up to https://github.com/rackt/react-router/pull/1724 and this gist, which formed the first draft of this PR.
This PR introduces a modular API that separates history listening, route matching, and component rendering:
// Create router by passing routes to route-creating function
// `createReactRouter()` is an enhanced version of base `createRouter()`
const router = createReactRouter(
  <Route component={Parent}>
    <Route path="/" component={Child} />
  </Route>
);

// Listen for location changes
// For server-side rendering, omit history and use `createLocation()` directly
history.listen(location => {
  router.match(location, (error, state) => {
    React.render(<RouterComponent {...state} />, el);
  };
});

Primary features of this API

Separate route matching and history listening allows for integrations with external state libraries, like Redux. It also makes server-side rendering easier — no special API needed, just pass a location object.
Separate rendering opens up the possibility of integrating with UI libraries other than React with minimal hassle, by sharing the core routing logic.
<RouterComponent> is a pure, dumb component (simply renders the app for a given set of props — no internal state whatsoever), replacing <Router> which is a stateful, smart component. Not sure about the naming, but I'm going with <RouterComponent> for now to avoid a name clash while both of these components exist in the codebase.
"Non-essential" routing features — like async component loading, transition hooks, and JSX routes — are implemented as router enhancers on top of a base createRouter() function. These are just like store enhancers in Redux, higher-order components in React, or decorators. Aside from making the library easier to maintain, they give flexibility to the consumer, and open up the possibility for some really cool userland extensions. We took this approach with Redux and the community response has been overwhelming.

For an example of how router enhancers work, take a look at createReactRouter(), which combines multiple enhancers into a single router creating function:
const createReactRouter =
  useComponents(
    useTransitionHooks(
      useJSXRoutes(
        createRouter
      )
    )
  );

So far, I've migrated over a single test file: Router-test.js -> RouterComponent-test.js. I'll probably hold off on migrating any more until I've received feedback.
Todo

[X] Base createRouter()
[X] Implement component loading, transition hooks, and JSX routes as router enhancers
[X] <RouterComponent> that renders for a given state returned by router.match()
[ ] <Link> functionality
[ ] Navigation and State mixins re-implemented as higher-order components
[ ] Convert all existing tests
[ ] Possibly re-implement <Router> on top of these primitives — would maintain 1.0 compatibility and make testing easier.

By the way, I have to mention that because of the great work that's already been done to move the core pieces of logic into pure functions (matchRoutes(), getComponents(), runTransitions(), etc.), it was a breeze to whip this PR together — I was able to focus entirely on API rather than having to re-implement/refactor complicated routing code. Really awesome work.

I'm really excited about this API.
Those changes are what RR needs to be the best router.

This all looks great, @acdlite! I really like how you're using the enhancer pattern we're already using in redux and history.
I'm really interested to see how the implementation of isActive and links is going to play out here. In my experience, those have been the trickiest pieces of the router to get right – but they're also where we provide some of the greatest value to users.

I have to mention that because of the great work that's already been done to move the core pieces of logic into pure functions (matchRoutes(), getComponents(), runTransitions(), etc.), it was a breeze to whip this PR together

I'm glad you feel that way :) I've done a lot of work over the past few months with an eye toward this API, trying to make sure all the right pieces were in place. Very glad to have some help to take it to the next level! :)
@vslinko I know you have some great ideas around how to do routing better as well. Please, feel free to get involved here :)

My ideas are around definitions of routes and transitions.
First of all, router should be framework-agostic. It means any object could be result of router work. I call that result Screen.
type Screen = Object

RR relies on declarative schema, but I suggest rely on single function, like redux does. I call that function Transition. Transition is async function that eventually returns screen or nothing.
type Transition = (location: Location) => Promise<?Screen>

That approach is very powerful. You could combine routes in any way:
var applicationTransition = createErrorTransition( // wrap child transition by try-catch
  combineTransitions( // call children transitions one by one
    homepageTransition,
    aboutTransition,
    prefixTransition('/admin', combineTransitions(
      dashboardTransition
    )),
    notFoundTransition
  )
)

You could create helpers for transition definitions:
type TransitionHandler = (queryParameters: {[key: string]: string}) => Promise<Screen>

// simple helper that match url and parse query params
var homepageTransition = createTransition('/', (params) => <HomePage name={params.name} />)

// helper that return transition that matches /SignInForm and /SignUpForm
var componentPreviewTransition = createPreviewTransition([SignInForm, SignUpForm])

// helper that return AboutPageContainer wrapped by Relay.RootContainer
var aboutPage = createRelayTransition(createTransition('/about', () => AboutPageContainer))

You could split application by modules:
var dashboardTransition = createTransition('/dashboad', () => {
  return new Promise(resolve => {
    require.ensure('./Dashboard', require => {
      resolve(require('./Dashboard'))
    })
  })
})

Sorry for my English :smile:

@vslinko That's a neat approach to transitions! One of the benefits of this API in this PR is you could swap out your own transitions API and still keep the rest of the router logic. See https://github.com/rackt/react-router/blob/modular/modules/useTransitionHooks.js, for example. Technically, you could even have multiple enhancers that implement transitions — doesn't necessarily need to be either/or.

BTW all my ideas are done in my router https://github.com/vslinko/vstack-router

Awesome work! I like where this is going.
One thing that strikes me is the removal of the transition object. Without it, is there any way to tell if a transition is superseded or otherwise aborted? For example it would be great to be able to abort getComponents (or any other async task) somehow when the history location changes.

@taurose I solve that problem in my library using synchronised queue.
Right now I'm just drop transition in middle of the queue because I'm not sure about how to safely cancel async action.

Also, my router has own subscription function that emits location and screen in sync, after transition is resolved.
Maybe it could be helpful for RR.

is there any way to tell if a transition is superseded or otherwise aborted?

There are two use cases here, AFAICT.

Some onEnter transition hook performs a redirect, in which case router enhancer functions get redirectInfo and can stop whatever async work they're doing and pass that back up the callback chain.
The user initiates another transition while the first is still in progress. This one is a little more tricky. In a client-side router that uses this architecture, you can just assume that whenever match is called you are transitioning to a new place and you can cancel any async work you're doing from previous calls. However, in a server-side router we should expect match to be called many times.

We can easily prevent old requests from triggering the callback using a simple enhancer, like this:
function mostRecentLocationWins(createRouter) {
  return (...args) => {
    const router = createRouter(...args);
    let pendingLocation;

    function match(location, callback) {
      pendingLocation = location;

      router.match(location, function (error, nextState, redirectInfo) {
        if (pendingLocation === location) {
          // We weren't interrupted. Call the callback!
          callback(error, nextState, redirectInfo);
        } else {
          // Another transition interrupted this one.
        }
      });
    }

    return {
      ...router,
      match
    };
  };
}

We should probably wrap all client-side routers in something like this. We could even generalize this function a bit more so that we could wrap other enhancers with it, for use client-side only.
function useComponentsClientSide(createRouter) {
  return (...args) => mostRecentLocationWins(useComponents(createRouter))(...args);
}

@vslinko I really like combining transitions in this way, and I agree that the whole thing could be done in a framework-agnostic way (just like we did the history module). Ultimately, your method of combining transitions could just produce a function that totally replaces our matchRoutes function.

Yes, in my case matching is transition responsibility.

@mjackson Yeah, that would certainly help. One problem I see though is that only the outermost enhancer is aware of the latest location (unless every enhancer calls router.match synchronously).
If we kept the transition as part of match we could write async tasks like before:
function match(location, transition, callback) {
  router.match(location, transition, (error, state) => {
    if (error || transition.isAborted) {
      callback(error);
      return;
    }

    doSomethingAsync(result => {
      // transition may have been superseded
      if (transition.isAborted) {
        callback();
        return;
      }

      callback(null, {...state, ...result});
   });
  });
}

On the server, the object could be created and handled yourself, and on the client there could be a router or history enhancer that creates the object, aborts it if superseded, and performs redirects.

Ya, you're right @taurose. A transition object would definitely be better.

One problem I see though is that only the outermost enhancer is aware of the latest location (unless every enhancer calls router.match synchronously)

I don't follow... why is this?

why is this?

I believe @taurose is referring to my mostRecentLocationWins enhancer above, @acdlite.
It solves the problem of the callback being called for old location objects by ensuring the callback isn't called if the location it is responding to isn't the most recent one that was sent to match. But it doesn't do anything to help enhancer functions in the middle of the stack know that the transition was interrupted, so they don't need to do async work. The transition object is a way for us to thread a common object down the stack so that enhancers can ask it at any time if they should bother proceeding or not.

@mjackson Ah, I get it now! Thanks for the explanation.
Is the transition object a change you want to get in before 1.0, or after? If before, then we should open up a separate PR.
If after, I think it's best to mark it the existing API as deprecated for a release cycle before changing it. My goal was to write this PR without breaking 1.0 compatibility — it may not be possible, but that's the ideal I'm striving for. Then we can deprecate and remove stuff in later releases, providing a nicer upgrade path for consumers, which tends to be more important after your first "stable" release. Does that sound reasonable to you?

Another thing that strikes is the current implementation of getState and how it's used in isActive and useComponents.
As far as I can tell, the router state is updated when the last enhancer overriding match calls router.match, which is before enhancers do their async work based on the router state like getComponents. So, when <Link>s are re-rendered while the router is loading the components for the next location, the <Link>s active state may end up reflecting the state of the upcoming location which the user couldn't even have rendered yet (correct me if I'm wrong :) ).
Also, with the way useComponents overrides getState I think it's possible for getState to return inconsistent results where the components are not in sync with the rest of the state.
My suggestion would be to get rid of getState which I think leaves two places where it was needed:

isActive/<Link>: could be solved by passing routerState via context instead of router, and using that state for calling  a pure isActive(routerState, pathname, query)
prevState for transition hooks: could be solved by including prevState as a match parameter, and having a router enhancer at the top of the stack that manages it. That would also make it possible to opt out for more control.

As far as I can tell, the router state is updated when the last enhancer overriding match calls router.match

When you call getState(), you're calling it on the last enhancer, which calls the next enhancer, which calls the next enhancer, and so on. It's impossible to call getState() directly on an inner enhancer.

My suggestion would be to get rid of getState which I think leaves two places where it was needed

We have to account for situations in the future, as well. If we get rid of getState(), the only way to get the current state is by calling match(), which is mutative. If anything, it may be a good idea to not pass state to match and always use getState(). (That's what we do with Redux — the reason we don't pass the current state to subscribers is because then if a store enhancer modifies getState(), it also has to modify subscribe(), which is easy to forget.)

could be solved by including prevState as a match parameter, and having a router enhancer at the top of the stack that manages it.

This is basically what the base createRouter() method does — in fact, that's all it does: https://github.com/rackt/react-router/blob/modular/modules/addIsActive.js. What you're describing is the pure function matchRoutes(), which already exists.

@taurose Although thinking about it more, you may be onto something about putting the state management stuff at the end of the chain rather than the beginning.
const router = createRouter(useComponents(useJSXRoutes(match)));

router.getState();
router.match();

I think I like this. Let's keep exploring this thought.

Okay, so then useComponents() and useJSXRoutes() are higher-order functions that enhance a base match() function. Welp, obviously we should call these middleware.
We'd still need a separate kind of extension that adds stuff to the store interface, rather than just wrapping match(). E.g. addIsActive(). Those will be router enhancers.
Oooh, this feels right!

Is the transition object a change you want to get in before 1.0, or after?

The transition object we discussed here is just an implementation detail, not part of the public API.

Okay, so I've gotten all but two of the tests to pass using the new primitives. The test files have not been adjusted at all, which is a testament to the overall thrust of this approach.
However, I've run into a few problem areas. The biggest by far is the relationship between history, query/path serialization, and the router. Right now we're in this weird place where the router is totally separate from the history, but both the router and the history object need to know how to read URLs.
Still working through this in my head, but here's an idea: what if createRouter() is a history enhancer? In this formulation, routers are a special type of history with additional methods:
// Most basic form
const router = createRouter(routes)(createHistory);

// Or compose enhancers to create special routers
const createReactRouter = compose(
  addNavigation,
  useJSXRoutes,
  applyMiddleware(
    useTransitionHooks,
    useComponents
  ),
  createRouter
);

// Usage
const reactRouter = createReactRouter(routes)(createHistory);
reactRouter.transitionTo('/some/path');
reactRouter.listen(state => {
  React.render(<RouterRenderer {...state} />, node);
});

createHistory() returns the same history object you already know; createRouter() adds match().
You can still swap out the history-creating function (createHashHistory, createMemoryHistory, etc.). But the final result is a single, unified interface, without the awkward separation we're dealing with now.
This goes along with the other change discussed earlier in the thread, which is to add the concept of middleware. A router middleware has a similar signature to middleware in Redux.
function middleware(router) {
  return match => {
    match(prevState, routes, location, (error, nextState) => {
      // do something...
    });
  };
}

// Use router enhancer to apply middleware
const createRouterWithMiddleware = applyMiddleware(middleware)(createRouter);

One remaining question is server-side rendering — we don't need a history on the server. Well, you don't need most of the features of a history, but you do need to be able to create hrefs (e.g. server-sider rendering of s), which is different for HashHistory vs BrowserHistory. Perhaps we can use a history enhancer on the server called freeze() that disables location updates and navigation but keeps the other functionality like path building:
// On the client
const router = createReactRouter(routes)(createHistory);

// On the server
const router = createReactRouter(routes)(freeze(createHistory));

Still thinking this through... I'm going to hold off on this PR for a bit so we can discuss. Lots to consider. For instance, I think we may be able to get all the same benefits I've just described, but with a more straightforward interface:
const router = createReactRouter(routes, history);

Any thoughts?
@mjackson @taurose

@mjackson I thought redirectInfo was exposed to users by Router.run() when doing server-side rendering.

what if createRouter() is a history enhancer?

Now THAT is a lovely idea :D
function createRouter(createHistory) {
  return function (options={}) {
    var history = enableQueries(createHistory)(options);

    var { routes, initialState: state } = options;

    // Just try and match. Don't mutate state.
    function match(location, callback) {
      matchRoutes(routes, location, callback);
    }

    // Try and match. Mutate state on success. Otherwise, handle
    // errors/redirects. Maybe public, maybe not.
    function dispatch(location, callback) {
      match(location, function (error, nextState, redirectInfo) {
        if (error) {
          callback(error);
        } else if (redirectInfo) {
          var { pathname, query, state } = redirectInfo;
          history.replaceState(state, pathname, query);
        } else {
          callback(null, state = nextState);
        }
      });
    }

    // Client-side API.
    function listen(listener) {
      return history.listen(listener, function (location) {
        dispatch(location, listener);
      });
    }

    return {
      ...history,
      match,
      dispatch, // users probably don't need this...
      listen
    };
  };
}

Use it like:
import createHistory from 'history/lib/createHashHistory';

var router = createRouter(createHistory)({ routes });

router.listen((error, state) => {
  React.render(<Router initialState={state} />, node);
});

Server-side we can use a router enhancer that knows about HTTP; how to interpret HTTP headers and issue responses. No need to disable location updates ... just return the appropriate response!
function createHTTPResponder(createRouter, response) {
  return function (options) {
    var router = createRouter(options);

    function pushState(state, pathname, query) {
      // TODO: Probably just redirect?
    }

    function replaceState(state, pathname, query) {
      response.writeHead(302, {
        Location: router.createHref(pathname, query)
      });

      // TODO: Maybe put state in a cookie?

      response.end();
    }

    function handleError(error) {
      response.writeHead(500);
      response.end();
    }

    function dispatch(request, callback) {
      // TODO: Derive location state from a cookie?
      var location = createLocation(request.url);

      router.match(location, function (error, state, redirectInfo) {
        if (error) {
          handleError(error);
        } else if (redirectInfo) {
          var { pathname, query, state } = redirectInfo;
          replaceState(state, pathname, query);
        } else {
          callback(state);
        }
      });
    }

    return {
      ...router,
      pushState,
      replaceState,
      dispatch
    };
  };
}

Use it like:
http.createServer(function (request, response) {
  var router = createHTTPResponder(
    createRouter(createHistory), response
  )({ routes });

  router.dispatch(request, function (state) {
    response.writeHead(200, {
      'Content-Type': 'text/html'
    });

    response.end(
      React.renderToString(<Router initialState={state} />);
    );
  });
});

I'm really, really excited about this direction. If you like it @acdlite, I'd be happy to add some real commits tomorrow :)

@mjackson I do agree that match() should be pure and not mutate state. Middleware should wrap this function. I don't know if I like how dispatch() has a different interface for server-side and client-side, though.
Perhaps rather than overloading a single method, client-side routers should use transitionTo() and replaceWith(), and server-side routers can use something like the dispatch() method of your createHTTPResponder(). These should be applied last in the chain (or close to last):
// Use transitionTo(), replaceWith(), pushState(), replaceState() like normal
const clientRouter = createRouter();
clientRouter.transitionTo('/some/path');

// Use getResponse()
// transitionTo() and the like will throw an error
const serverRouter = http(createMyRouter)();
serverRouter.getResponse(req, (res) => {
  // do something;
});

I don't know if I like how dispatch() has a different interface for server-side

It doesn't have to be different. Think of createHTTPResponder as more of an "adapter" that is specific to node HTTP servers. It's the piece that ties the router in with node's domain, so it's ok that they're closely coupled. It would probably live in a separate package, even. We would probably have separate adapters for express and hapi as well.
The point is, we don't need to freeze these server-side "history" objects. Yes, it's true that there is no history on the server, but calling pushState/replaceState conveys intent that we can still communicate back to clients through the HTTP interface.

client-side routers should use transitionTo() and replaceWith()

I'd actually like to get rid of these methods. If we're just going to wrap createHistory, then we already have pushState/replaceState, so they're redundant.

The point is, we don't need to freeze these server-side "history" objects. Yes, it's true that there is no history on the server, but calling pushState/replaceState conveys intent that we can still communicate back to clients through the HTTP interface.

Say you call pushState() on the server. What happens?

I'd actually like to get rid of these methods. If we're just going to wrap createHistory, then we already have pushState/replaceState, so they're redundant.

I'm cool with that as well.

Say you call pushState() on the server. What happens?

Honestly, I'm not 100% sure about pushState because it inherently means "add an entry to the history" which is meaningless in a stateless environment. But pushState still conveys intent. When you call pushState you essentially mean "go here". In a server-side environment, I'd probably do something like:
pushState(state, pathname, query) {
  warning(
    false,
    'You cannot pushState() in a server environment, using replaceState() instead'
  );

  replaceState(state, pathname, query);
}

replaceState is easy–we just redirect. In any case, I think we can handle it gracefully and teach users not to do that.

Pretty hard to catch up here ;) Moving createRouter to the end of the chain like that could solve many of the concerns I had.
My intention was to make the transition object part of the match API though and use it like this
  var transition = null;

  function dispatch(location, callback) {
    if (transition)
      transition.cancel();

    transition = createTransition();
    matchRoutes(routes, state, location, transition, function (error, nextState) {
      if (error) {
        callback(error);
      } else if (transition.redirectInfo) {
        var { pathname, query, state } = transition.redirectInfo;
        history.replaceState(state, pathname, query);
      } else if(!transition.isCancelled) {
        callback(null, state = nextState);
      } else { /* superseded */ }
    });
  }

I actually hadn't noticed that transition was removed before this PR. If you want to get rid of it, I think there should be some other way to cancel the current match run and let the middleware/hooks know.

We have to account for situations in the future, as well. If we get rid of getState(), the only way to get the current state is by calling match(), which is mutative.

Not sure what you mean with situations in the future. The current state would be managed by the user and passed to the React component as he pleases. Middleware could still access prevState and nextState parameters.
I'm not sure how I feel about having to use history on the server, and using pushState/replaceState for redirects. I'm thinking the client router should build upon the server-side router rather than the other way around, in order to add transition management, integrate history and perhaps maintain state. Then the server-side router would just have to provide a pure match and a createHref. HttpResponder and location utilities could be provided as middleware.
var router = createRouter({routes, createHref: path => path});
var handleRequest = (request, response) => {
  router.match(createLocation(..), state => {
    var html = React.renderToString(<RouterRenderer {...state} router={router} />);
    ...
  });
};

Second attempt, made some good progress. (In modules/take-2 for now.) createRouter() and createReactRouter() are now history enhancers (they feel very much like higher-order components).
export default function createReactRouter(createHistory) {
  return options => {
    const { middleware, routes } = options;

    const nextOptions = {
      ...options,
      middleware: composeMiddleware(
        middleware || identityMiddleware,
        useTransitionHooks,
        useRoutes,
        useComponents
      ),
      routes: createRoutes(routes),
    };

    return compose(
      addNavigation,
      createRouter,
      createHistory
    )(nextOptions);
  };
}

Still some work to do; I'll pick it up again tonight.

@acdlite I don't want to distract you but any chance you could also do https://github.com/rackt/react-router/issues/470#issuecomment-113305163? This way <Link> won't rely on context change propagation.

@gaearon Sure, thanks for reminding me about that.

Okay, I got all but one of the tests passing! (Has to do with state.location.pathname including a hash when using HashHistory... not sure what the correct behavior is supposed to be here?)
The public-facing API is the same — I haven't modified the tests at all. Internally, the router is creating using a collection of history enhancers. The most important is createRouter():
const router = createRouter(createHistory);

I was calling this addRouting() for a while, but eventually came back around to just createRouter(). Not 100% sure about the naming
Routers are given a pure match(prevState, location, callback) function. Think of it as an async version of a reducer in Redux, where location is an action and instead of returning a new state synchronously, it uses a callback.
Routers also have the concept of middleware. Middleware is passed as an option. They are higher-order functions with the following signature:
routes => match => newMatch

Most of React Router's functionality has been implemented as middleware: route and param matching, transition hooks, and async component fetching. You can use composeMiddleware() to combine multiple middlewares together.
For an idea of how this all fits together, take a look at the implementation of createReactRouter():
export default function createReactRouter(createHistory) {
  return options => {
    const { middleware, routes, location, ...rest } = options;

    const nextOptions = {
      ...rest,
      location,
      // Here are our middlewares
      middleware: composeMiddleware(
        location ? matchUntilResolved : identityMiddleware,
        middleware || identityMiddleware,
        useTransitionHooks,
        useRoutes,
        useComponents
      ),
      routes: createRoutes(routes),
    };

    // These are normal enhancers
    return compose(
      addNavigation,
      addIsActive,
      createRouter,
      useQueries,
      addSingleLocationSupport,
      createHistory
    )(nextOptions);
  };
}

I feel pretty good about this set-up. Excited to see what you've been working on as well, @mjackson!

Clarifying this comment:

The public-facing API is the same — I haven't modified the tests at all. Internally, the router is created using a collection of history enhancers.

If the public API is the same, what's the point of this PR? If we expose the internals, or move them to a separate project, we can allow people to customize router behavior with their own enhancers and middleware. Then, instead of relying on <Router> to manage state for them, they can use <RouterRenderer> directly with whatever state management system they like. E.g. redux-react-router can use an api like this:
const router = createReactRouter({ routes })(createHistory);

// Use like any other store enhancer
const store = compose(
  reduxRouter(router),
  applyMiddleware(thunk, promise),
  createStore
)(reducer, initialState);

// Now the router state is in your store
// Access query, params, pathname, etc. with normal Redux selectors
store.subscribe(state => {
  // Debounce router state here somehow
  // May also provide a <ReduxRouter> component that takes care of this for us
  React.render(<RouterRenderer {...state.router} />, node);
});

+1 loving this

+1 xD

@acdlite awesome work! I'm really looking forward to this clear separation of modules! :+1:

@acdlite Great job! Look forward to having this in main branch.

+1!

Excellent work here @acdlite! (even if I am having a bit of a struggle understanding how it's all pieced together...)
I ran with your idea to make a "router" just another history enhancer function in the history-enhancer branch. Most of the functionality is contained in the useRoutes function which adds match, isActive, and a few other methods to a history object.
One of the major new pieces of functionality that I have working there is the ability to set transition hooks per route. This is a tricky one to implement, but it essentially lets users use a routerWillLeave lifecycle method that is called when the route that renders that component may transition out. It's a way to cancel/confirm the transition on a per-route basis. This was one of the last main features I needed to get in to the router before 1.0.
In order to implement this, I had to implement a two-phase match. Here's why: in order to know which routes we are leaving (this is before listen is notified, during the "confirm navigation" phase), we need to do a match and compare with the new branch. However, as you know a match may involve fetching new routes, which is work that we don't want to do again if we do actually confirm the transition and decide to go through with it. So I cache the result of matchRoutes locally and pickup where I left off when the location actually comes through in the listen callback.
I'm going to work to get all tests passing on the history-enhancer branch and add a test for routerWillLeave as well so hopefully you'll have something to work with if you'd like to support this feature in this branch as well.

The public-facing API is the same — I haven't modified the tests at all.

Really makes me wish we had better tests :)

Here's what i'm doing with 1.0.0 beta3:

Can't wait to not be redirected by a component xD

Sorry I've been off the grid for a few days; been busy with other stuff. I saw the new changes in v1.0.0-beta4 — not exactly how I would have done it, but it's a great improvement!
I'm going to close this PR then since I believe it has served it's purpose, but please don't delete the branch @mjackson. If you ever decide to move the non-React stuff into a separate module a la history, I think there are ideas in here that will be useful.

@acdlite Thank you so much for creating this branch and having this discussion. The useRoutes function was entirely inspired by your suggestions here. I do still think this branch can be useful, so I definitely won't take it down. :)
Let's keep working together, and hopefully get to a place where we can both feel comfortable with the architecture. Again, thank you so much for your work here.

@mjackson Thank you for being so accommodating! Sure thing, I'm looking forward to seeing where this goes in the future.
I'm going to update redux-react-router to use the new API. I think having a third-party project that uses these new internals will help identify possible areas for improvement.

It's quite a bit after the fact, but I'm curious where we stand here - are there still ideas from this PR that we want to bring into the main line?
Are there any practical lessons we've learned in building integrations on top of React Router so far that add color to this discussion?


Column resizing

I've seen a request from 2016 requesting a feature for column resizing, much like is implemented with http://www.bacubacu.com/colresizable/, but the request was closed.
Is this feature planned, can it be added to the list, or is it already implemented?
Thanks for a great project.

Hi @markcs, this feature is not planned at this stage as very few requests for the amount of effort involved in this tricky exercise :).
Have you tried to integrate a column resizable widget with TableFilter?


[Bug] flink写入异常

Search before asking

[X] I had searched in the issues and found no similar issues.

Version
2.0.1-rc03
What's Wrong?
flinksql消费kafka写入doris，偶发no available backends

What You Expected?
正常插入数据
How to Reproduce?
No response
Anything Else?
No response
Are you willing to submit PR?

[ ] Yes I am willing to submit a PR!

Code of Conduct

[X] I agree to follow this project's Code of Conduct

Please provide the log information after be crashed.


LAB_09c-Implement_Azure_Kubernetes_Service.md

Current Translation:
Contoso memiliki sejumlah aplikasi multi-tingkat
Suggested Translation:
Contoso memiliki sejumlah aplikasi multitingkat

Hello contributor,
Thank you for your contribution! We are processing your suggestions and we will update you shortly.
Kind regards,
Microsoft Worldwide Learning Team

Run the bugfix tool and waiting for result. ICMS QUERY


Externalize all but the largest third_party/go packages [phab:D852]

Created by schroederc at 2016-04-21 17:17:11:

shahms accepted this revision at 2016-04-21 17:23:47

schroederc closed via commit 64371124d425ed5e2a2bcaa5876123962dbff6a0 at 2016-04-21 17:24:18