Loading spinner disappears after first render, then reappears

Describe the bug
After clicking the Run Model button, you are taken to the results screen /results. A loading spinner appears while the server is working, but then it disappears and text that shouldn't appear while a model is running appears. Then the spinner reappears.
To Reproduce
Steps to reproduce the behavior:

Go to https://musicbox.acom.ucar.edu/
Click on Get Started
Click on Select under Chapman Cycle
Click on Run Model
Observe the flicker

Expected behavior
The spinner should be displayed the entire time the model is running
Screenshots

Additional context
Important files:

src/components/Layout.jsx, this is where the Run Model button is. The run click handler dispatches an UPDATE_RUN_STATUS event, and calls doRun which is in src/redux/actions/run.js
src/redux/actions/run.js, Translate the current configuration to what the server needs to run and calls the run api endpoint
src/controllers/api.js, this has the run call
src/redux/middleware/polling.js, this is where we poll for the status and dispatch UPDATE_RUN_STATUS events
src/redux/reducers/results.js, handles updating the redux store with the new run status
src/pages/results.jsx, handles displaying the spinner

Run Model workflow
In general this is what happens when you poke run, to the best of my ability to understand what happens.
Click "Run Model" -> Reset the plot information -> Set the run status to Running -> Call doRun -> Call run on the API server -> Dispatch a START_POLLING event -> The polling middleware should receive the START_POLLING event -> navigate to /results
What may be happening is that the status gets set to RUNNING by the Run Model callback, then updated again to WAITING in the first response to check the results in the poll function, then the next polling event sets it to RUNNING and the spinner updates. Indeed, if you open up the chrome console and look at the response for run-status, the first is WAITING and the second is RUNNING. So, what probably needs to happen is that callback fro RUn Model needs to set the status to WAITING
However, it would be nice for the spinner to show always after clicking Run Model until an error or results come back
To run the server to get model results, see our readme

Is this issue resolved?  I can't reproduce the bug.
Screencast from Saturday, 23 December, 2023 12:00:22 PM PST.webm

@iamwesofph you don't seem to be connected to a dev API server or the production one. Without that, you won't actually be getting any results back and wouldn't see the issue. Please check the chrome console; you'll likely see the API calls fail. Our readme states how to setup with either a dev or production API server. However, if you need help, I can provide more insturctions or guidance here. Thank you for your time!

I actually setup the dev server using:
VITE_API_URL='https://musicbox.acom.ucar.edu/musicbox'
Screencast from Saturday, 23 December, 2023 01:55:49 PM PST.webm

@iamwesofph. If you are connected to the prod server, when you click on the chapman example you would see a list of species appear, like in the gif in the issue. I've confirmed that this is still an issue for me client side hitting the production server. I'm unclear about why the server connection would fail for you, likely an nginx configuration or something that's preventing this...
If you are interested, you could setup a development server on your machine:

Change VITE_API_URL back to http://localhost:8000
git clone https://github.com/NCAR/music-box-interactive-api.git
cd music-box-interactive-api
docker compose build && docker compose up -d && docker compose logs -f

However, I understand if that seems like too much work.

@iamwesofph I did check our metrics and appear to see activity that looks like what is in your screencast. It appears there's some bug preventing you from seeing the results of the API calls, but I'm unclear why I don't see the same behavious as you.
As an alternative to setting up a local dev server, and if since you seem to be connected to the prod server, you could setup a simple reaction yourself while connected to the prod server.

Click Get Started
Click Start from scratch
Enter A into the species box
Click on Reactions -> Emission
Click on the <none> -> <none> text
Next to species click on  and choose A
In the sidebar, click on Conditions
Click on Initial
Add the bottom, in the Reaction Rates and Rate Constants click on the +
Click on the select box under Reaction Label
Choose  -> A
Give it an Initial value of 0.01
Give it units of  mol m-3 s-1
Click run

Then you will hopefully see the issue and also have run your first atmospheric box model chemical system

Hi, what do you mean server connection failed me?  Also, what results of the API calls am I not seeing?
Im just trying to replicate the bug issue where the spinner flickers because it shows this part:
const ResultsNotStarted = () => {
return (
<>
To run a simulation, click the green Run button to the left
The possibilities are limitless.
</>
);
};

I was also following your steps you outlined but I cant find these:
Click on Conditions Click on Initial Add the bottom, in the Reaction Rates and Rate Constants click on the + Click on the select box under Reaction Label Choose -> A Give it an Initial value of 0.01 Give it units of mol m-3 s-1

@iamwesofph apologies, I didn't' realize my previous comment was improperly formatted.
You've correctly added an emissions reaction. In the left sidebar, there's a link to the Conditions page. On that page you'll see a tab called Initial. At the bottom of the Initial tab is where you'll find the Reaction Rates and Rate Constants.
I've fixed the instructions above, hopefully you can get it from there. Thanks for trying this out!

Hi, what do you mean server connection failed me? Also, what results of the API calls am I not seeing?
Im just trying to replicate the bug issue where the spinner flickers because it shows this part:
const ResultsNotStarted = () => {
    return (
        <>

To run a simulation, click the green Run button to the left

The possibilities are limitless.

        </>
    );
};

After you click on the Chapman example, you should see a number of species besides M. You can check in the network tab of chrome if the load-example call returned any json. The output is too long to post, but it should be ~ 319kB of information. If you do see json returned, then for some reason the client side is not populating the redux store for you, which is odd.

If you try, the list doesn't load when you click the mechanism tab at the left.
This could be another issue if it's not the intended way your app works.  It improves user experience if the user does not have access to confusing mechanism (like when the user shortcuts to clicking "mechanism" instead of starting from "start here").

Screencast from Sunday, 24 December, 2023 07:46:38 AM PST.webm
Unfortunately, I still can't reproduce the bug issue you're facing.  I tested this multiple times using npm run dev, npm preview, and the link https://musicbox.acom.ucar.edu/
Have you tried using another workstation and browser to test this?

Even though I cannot replicate the bug, its quite clear what is causing the flickering issue you're describing (as I am seeing from the very quick GIF you attached).
This is the related code:
    <Layout>       <Container className="jumbotron text-center hero-img">         {(() => {           switch (runStatus) {             case RunStatus.RUNNING:               return <ResultsRunning updateCount={renderCount.current} />;             case RunStatus.DONE:               return <ResultsDone />;             case RunStatus.ERROR:               return <ResultsError errorMessage={error.message} />;             case RunStatus.WAITING:               return <ResultsNotStarted />;             case RunStatus.NOT_FOUND:               return (                 <ResultsError errorMessage="Unexpected server error. Please try your run again." />               );             default:               console.error(Unknown model run status: ${runStatus});               return <ResultsNotStarted />;           }         })()}       </Container>     </Layout>
On your case, this is the RunStatus' status:
RunStatus.RUNNING >> RunStatus.WAITING >> RunStatus.RUNNING >> RunStatus.DONE

@iamwesofph there are three examples the users can choose from. Going straight to the mechanism tab works only if you want to build up the mechanism by hand.
I've seen this issue on two separate computers, and my coworker has also mentioned it. Perhaps it's a difference in internet speeds or something?
As for why we we have the waiting state, the chemical systems themselves can take quite awhile to run and we only have the capacity to run three at a time. When you click the run button, your data gets placed into a queue of models to run and may not be run immediately. In fact, we've seen this when we tested with ~10 scientists at one time.
I believe the bug is because the Layout component initially sets the state to RUNNING, the server can respond and say the status is actually waiting, so the Layout component shouldn't assume we will start in a RUNNING state and should instead set the status to WAITING.
However, you're correct that the results screen doesn't actually show any useful information when it's in a waiting state. Would you consider fixing the bug in the Layout component and, when on the results page and in the WAITING state, showing something like "We've queue your run request! A loading indicator will appear when your box model begins its simulation" or something like that.

Perhaps it's a difference in internet speeds or something?

I don't think so, I tried using Slow 3G speeds.  As long as the API returns a WAITING state, then the message will display therefore the flicker effect.

A loading indicator will appear when your box model begins its simulation" or something like that.

Can you be specific of what kind of loading indicator you want to see?  From my perspective, as a user, I just want to see the spinner if the app received my command. But its my opinion.

However, it would be nice for the spinner to show always after clicking Run Model until an error or results come back
I'm more inclined to make this work for you instead, if you agree. But first I want to recreate the bug.  Do you have an idea on what input might cause a WAITING state?

Just so were on the same page, this is what WAITING state displays:

Do you really need this?  Because the user already clicked the run button.
Also if I make the app display "We've queue your run request!", when will it be transitioned into a spinner?

@iamwesofph that text is not needed for sure. It would be better to have something that indicates the difference between a simulation waiting to run and then the spinner which is when it's actually running.

@iamwesofph a waiting state is sent back by the server when the simulation is waiting to run
Here are three screen shots that show the results of api/run and api/run-status

@iamwesofph that text is not needed for sure. It would be better to have something that indicates the difference between a simulation waiting to run and then the spinner which is when it's actually running.

Isn't this the indication that the simulation is waiting to run?
Thanks for sending the screenshots of the network tab.  The first WAITNG status coming from "run" does'nt really generate the waiting message (To run a simulation click the green button on the left).  The second WAITING status from the second picture coming from "run-status" is the one generating the waiting message, and that is also the one data I still cannot replicate/reproduce.  So I'm looking at the backend/server API now to check the logic of the code.
My initial thoughts and gut feel tells me that the run-status API is unnecessary, the original "run" API can simply return data that tells the frontend that the simulation has finished, eliminating the need for doing multiple HTTP requests ("run-status) polling for the status of the original HTTP request ("run).  I may be wrong though, so I will check the codebase first but calling an HTTP request every second seems fishy to me.

@iamwesofph The chapman model is relatively small and doesn't take too long to finish. If you run the full gas phase example, you should have to wait several seconds before results appear. After pressing run, results won't be returned immediately, so we do need to poll the server (at least until we implement #208). These simulation times can be controlled on the condition page; adding more fine-grained time steps or increasing the total runtime of the simulation will lengthen the time to results.
This simulation runtime is not something that the API server or the client code can control. We will in the hopefully not so distant future be switching to a different ODE solver in our box model code, which should speed things up noticeably. However, even with that, results won't be immediate.

Hello, can you help me reproduce this issue?  How do I run the full gas phase example?  I already tried this instruction that you gave me, but it didnt reproduce the issue:

Click Get Started
Click Start from scratch
Enter A into the species box
Click on Reactions -> Emission
Click on the  ->  text
Next to species click on  and choose A
In the sidebar, click on Conditions
Click on Initial
At the bottom, in the Reaction Rates and Rate Constants click on the +
Click on the select box under Reaction Label
Choose  -> A
Give it an Initial value of 0.01
Give it units of mol m-3 s-1
Click run

Hi @iamwesofph , the third example on the getting started page is the full gas phase example.

You can also verify that you're getting results by navigating to the plots page. For the Chapman example, plotting O1D should show you peaks and troughs.

I ran the full gas phase and got around 20+ "run status" network calls and I didn't get a single response with "WAITING".  Every single request returned a "RUNNING" response, and then finally a "DONE" response.
So again, I cannot reproduce the bug, which is the first step before I can fix the issue.

@iamwesofph it does not always happen this way. You appear to be lucky enough to never have the waiting status, but it can happen, as evidenced by my screenshot earlier.


Hi, you requested feedback on my comment on the marketplace.

Marketplace comments

I wrote - "Good work! The Firefox extension works really well with the VS Code extension.
It took a while before I realized how to set my settings, but now that I get it, I'm thrilled."
You respond - "Hey Mikael Sundström!  Thanks a lot for the try and feedback.
I understand the browser extension is little bit confusing!
Would you like edit the documentation or make a tutorial so that others can get it very easily ? (and yes, if you've free time)"

Issue
First of all, I did not know if PHP was running in connection with the "Go Live"-button and whether it was self-hosted by the extension. So I had to figure it out first. I did not know if it would be as easy as .html-document, and I did not know what the meaning of the browser extension was.
The biggest misunderstanding was the placeholder in the browser extension for the Actual Server Address: http://localhost:80/app. I did not understand if /app was default and if the VS Code extension created a symbolic environment in the /app directory. I tried to visit both http://localhost/app and http://localhost/, without successful results.
In the end, I tested to fill in the field with http://localhost/ for Actual Server Address.
I run Apache with AppServ on standard port and then I moved the workspace folder to C:\AppServ\www\my_workspace.
My browser extension settings is:

Actual Server Address: http://localhost/
Live Server Address: http://<IP_ADDRESS>:5500/

I think these settings should be standard and if you need other settings, enter them.
My knowledge
I am an amateur/hobby programmer and my understanding of programming concepts and workflow might not be accurate and correct. There may also be handling errors from my side.

hey @Mikael-Sundstrom, Thanks a lot for the response.
I under about the placeholder, I'll change it.
But I need a person who will write good docs or youtube video. https://github.com/ritwickdey/live-server-web-extension/issues/4.
I've build the extension. So, I know clearly how to setup it. But you were fresh mind and didn't know how to setup it and finally after few times of try, you able to setup it.
This is why I'm asking you (if you've time), would you like to like help me to write docs or make a new youtube video. [Honestly, my english is not good]
[The repo of the browser extension is here https://github.com/ritwickdey/live-server-web-extension]
Thanks Again!

If you're interested & if you need help to do it, I'll help you 200% personally 😃


pallet-balances: Decouple existential deposit and kill account

The situation
Acala is using pallet-balances and orml-tokens at same time to handle tokens. pallet-balances is used to handle the native token ACA and orml-tokens is used to handle other tokens (e.g. aUSD, renBTC, DOT, etc).
We have implemented a solution to allow user to use any supported tokens (e.g. aUSD, renBTC, DOT) to pay for transaction fee. This means our users does not need to hold our native token ACA at all.
To prevent dust account, pallet-balances implements a way to wipe account data (balance & nonce & refcount) when account balances drops below existential deposit.
The problem
We may not want to wipe account data when the pallet-balances balance drops below existential deposit because the user may hold other tokens and therefore should not remove nonce.
However the current implementation of pallet-balances will always drop account data when balances drops below existential deposit.
We can prevent balances drops below existential deposit by increase ref count for accounts have orml-tokens balances but because it is totally possible for an account never have any ACA balances at all, those account cannot be killed. Decrease ref count to zero does nothing at current Substrate implementation.
The changes we need
There are few changes required to solve our problem and not introduce breaking changes to existing production chains

Add a hook to frame-system when ref count changes (or drop to zero)

This will allow us to trigger kill account when ref count drops to zero

Add a hook to pallet-balances when an account balances changes (or when changing existential deposit status)

A struct will be provided to have the existing behavior. i.e. prevent drop to zero if not allow death and kill account if drop to zero
We will make it inc/dec ref count for our chain

As a result, on Acala, an account cannot be killed as long as it holds any kind of tokens. When the balances for all tokens drops below existential deposit, then the account will be killed on last transfer.

Will be fixed by https://github.com/paritytech/substrate/pull/7363


auro-hyperlink: nonPropagation of click event on hyperlink

Is your feature request related to a problem? Please describe.
When an auro-hyperlink is nested in an auro-checkbox, clicking on the hyperlink causes the checkbox to be toggled.
Describe the solution you'd like
Clicking on any other text inside of the checkbox slot should toggle the checkbox but not clicking on the hyperlink.
Describe alternatives you've considered
We are catching the events and overriding event propagation but feel like stopping events from propagating outside of an auro-hyperlink might be a good thing to have baked in.
Additional context

Click on stepout link
New tab opens & checkbox updates to this:

Related to Issue: https://github.com/AlaskaAirlines/auro-checkbox/issues/25

Who is a design sponsor for this work?

Kaitlin Reiss is the designer for NFP.

There is a way to address this UI without having to reshape the auro-checkbox element.
<auro-checkbox id="computers" name="checkboxDemo6" value="computers" style="display: inline-block; margin-right: -5px">
  I understand this is a non-refundable purchase and agree</auro-checkbox>
  to the Alaska Lounge
  <auro-hyperlink href="/" target="_blank">
    terms and conditions
  </auro-hyperlink>

I feel that this is an acceptable approach to the problem without having to build in special case support into a primitive element.

@rallyb13 you down?

Here is my long-vision opinion on this.
We should NOT be using this visual pattern due to issues with a11y. What we SHOULD be doing is embracing the newer modal of acceptance that are currently using with our Cookie Policy banner.

This issue and the duplicate https://github.com/AlaskaAirlines/auro-checkbox/issues/25 will be closed as there are no proposed updates to be done with this or the auro-hyperlink primitive elements.

@sk8lin?

Cross-posting from duplicate https://github.com/AlaskaAirlines/auro-checkbox/issues/25: Just for future reference, a possible workaround to this is to use the onInput event instead of onClick. The hyperlinks do not seem to trigger the checkbox when I use that event.


covariates including age sex (and cohort)? intuitively, are the these covariates affect the result a lot?

In the paper,  "donor’s age as numerical covariate in r and the tissue type, sex and cohort". Just intuitively (I mean I am asking exact numeric numbers), do they affect much? On my side, I use covariates majorly of 5362 SNPs, then with "SEX" "AGE" "DTHHRDY" in GTEx_Analysis_v8_Annotations_SubjectPhenotypesDS.txt.
On my side, even in regression algorithms (let alone In a neural network on my side), the effect is small or unexplainable (well anyway, I just focus on PROTEIN.CODING.GENE on chrome).

It depends on the covariates. Some covariates such as sex or age are known to be associated with gene expression levels, so I expect that including them in the model will improve the imputation quality.
It would be really interesting to perform an ablation study to see which covariates are actually helping.

It depends on the covariates. Some covariates such as sex or age are known to be associated with gene expression levels, so I expect that including them in the model will improve the imputation quality.
It would be really interesting to perform an ablation study to see which covariates are actually helping.

Oh, no need to provide me an ablation study ... (you take 9 hours or even days to run) just in my situation (focusing on regression on SNPs) it does not help a lot. Just to make sure you also do with these covariates simply by adding them into features; I mean nothing else.
Thanks a lot, you response is already helpful enough.


Docs and refactoring

This PR is mostly an overhaul of the entire edoc.
The join/3 function has been removed in favor of append/3 and prepend/3, and together with join/2 learned some new tricks ;)

Whoa :) Nice work, but it will take me literally ages to proofread this =^^=

it will take me literally ages to proofread this =^^=

What are you waiting for then, get to it ;)))

What are you waiting for then, get to it ;)))

Yeah, sure ;) Already on it.

Thanks :)


Direct upload to Amazon S3: corrupted files

Hello everyone !
I'm currently trying to setup a direct upload from angular-file-upload to Amazon S3 bucket.
The upload process works fine, ex: If I upload an image of 3MB, the image is correctly transferred on my Amazon S3 bucket (located in Francfort btw, using the new AWS V4 signature)
The issue is the following one: The uploaded files cannot be opened and when I try to "cat" to see the binary content of the file, there are 3 lines added:
------WebKitFormBoundaryfOtaE6TQBF0hzBnw
Content-Disposition: form-data; name="file"; filename="01_-_Rock_Or_Bust.mp3"
Content-Type: audio/mp3
For each uploaded file, there is this "webkitformboundary" stuff added inside my file's binary content which is the reason of my issue I guess.
Does anyone know how to solve it?
I have a Node.js server that is basically generating pre-signed url and my app using this plugin to do the upload to S3.
I've been looking to solve this with my team for 24 hours now but no clue at moment.
Any ideas?
Best regards,

This is the issue tracker for Fine Uploader, not angular-file-upload.


query error from HUDI external table with parquet file format and lzo compression

Steps to reproduce the behavior (Required)

create hudi table with parquet file format and lzo compression, and insert data:

HUDI_CONF:
 hoodie.table.base.file.format=PARQUET
 hoodie.parquet.compression.codec=LZO

 create table hudi_parquet_lzo(
 uuid int,
 col_boolean boolean,
 col_int int,
 col_long long,
 col_float float,
 col_double double,
 col_decimal decimal(38,18),
 col_date date,
 col_timestamp timestamp,
 col_string string,
 col_binary binary,
 col_array array<int>,
 col_struct struct<a:string,b:int>,
 col_map map<string,int>
 ) USING hudi
 TBLPROPERTIES(
 type = 'cow',
 primarykey = 'uuid');

 insert into hudi_parquet_lzo select 1,true,1,1.01,1.001,1.0001,10000000000.0000000001,cast('2020-01-01' as date),cast('2020-01-01 00:00:01' as timestamp),'Top 10 Unsolved Mysteries of Paleontological Dinosaurs, Did You Know?',cast('1110001010101011001001' as binary),array(1,10,100),null,null;
 insert into hudi_parquet_lzo select 2,false,2,2.02,2.002,2.0002,20000000000.0000000002,cast('2020-02-01' as date),cast('2020-02-01 00:00:02' as timestamp),'Xi Jinping, general secretary of the Communist Party of China Central Committee, called on Tuesday for unrelenting efforts in exercising full and rigorous governance over the Party, saying that the CPC will continue to show zero tolerance for corruption.',cast('1110001010101011001001' as binary),array(2,20,200),null,null;
 insert into hudi_parquet_lzo select 3,true,3,3.03,3.003,3.0003,30000000000.0000000003,cast('2020-03-01' as date),cast('2020-03-01 00:00:04' as timestamp),'Nation raises caution on overseas packages',cast('1110001010101011001001' as binary),array(3,30,300),null,null;
 insert into hudi_parquet_lzo select 4,false,4,4.04,4.004,4.0004,40000000000.0000000004,cast('2020-04-01' as date),cast('2020-04-01 00:00:04' as timestamp),'Shanghai, Shenzhen register most newly listed firms in 2021',cast('1110001010101011001001' as binary),array(4,40,400),null,null;
 insert into hudi_parquet_lzo select 5,true,5,5.05,5.005,5.0005,50000000000.0000000005,cast('2020-05-01' as date),cast('2020-05-01 00:00:05' as timestamp),'A total of 4,685 companies have been listed on the A-share market in China as of Dec 31, 2021, with 46 percent of them based in Beijing, Shanghai, Shenzhen, Hangzhou, Suzhou, Guangzhou, Ningbo, Nanjing, Wuxi, and Chengdu, said a report of National Business Daily on Wednesday.',cast('1110001010101011001001' as binary),array(5,50,500),null,null;
 insert into hudi_parquet_lzo select 6,false,6,6.06,6.006,6.0006,60000000000.0000000006,cast('2020-06-01' as date),cast('2020-06-01 00:00:06' as timestamp),'The development of enterprises, as well as the push of local governments, has boosted the listings, the report said. For example, in early 2018, Central China''s Hubei province launched a plan to double its listed companies by including the listing of enterprises into the government''s annual performance assessment.',cast('1110001010101011001001' as binary),array(6,60,600),null,null;
 insert into hudi_parquet_lzo select 7,true,7,7.07,7.007,7.0007,70000000000.0000000007,cast('2020-07-01' as date),cast('2020-07-01 00:00:07' as timestamp),'Most listed companies in the top 10 cities come from industries such as information technology, electronics, mechanical equipment, medical biology, and electric power equipment, with information technology bearing most listed companies.',cast('1110001010101011001001' as binary),array(7,70,700),null,null;
 insert into hudi_parquet_lzo select 8,false,8,8.08,8.008,8.0008,80000000000.0000000008,cast('2020-08-01' as date),cast('2020-08-01 00:00:08' as timestamp),'Citi report finds MNC mood in China buoyant',cast('1110001010101011001001' as binary),array(8,80,800),null,null;
 insert into hudi_parquet_lzo select 9,true,9,9.09,9.009,9.0009,90000000000.0000000009,cast('2020-09-01' as date),cast('2020-09-01 00:00:09' as timestamp),'Xi: China, Russia major champions of multilateralism, global justice',cast('1110001010101011001001' as binary),array(9,90,900),null,null;
 insert into hudi_parquet_lzo select 10,false,10,10.10,10.010,10.00010,100000000000.00000000010,cast('2020-10-01' as date),cast('2020-10-01 00:00:10' as timestamp),'Volkswagen deliveries hit by chip shortages',cast('1110001010101011001001' as binary),array(10,100,1000),null,null;
 insert into hudi_parquet_lzo select 11,true,11,11.11,11.011,11.00011,110000000000.00000000011,cast('2020-11-01' as date),cast('2020-11-01 00:00:11' as timestamp),'Light installations featuring the tiger, the Chinese zodiac animal for the upcoming lunar year, will be set up in the main plaza.',cast('1110001010101011001001' as binary),array(11,110,1100),null,null;
 insert into hudi_parquet_lzo select 12,false,12,12.12,12.012,12.00012,120000000000.00000000012,cast('2020-12-01' as date),cast('2020-10-01 00:00:10' as timestamp),'The annual lantern show at Yuyuan Garden, a historic tourist site in Shanghai, will kick off on Tuesday and run through Feb 28.',cast('1110001010101011001001' as binary),array(12,120,1200),null,null;
 insert into hudi_parquet_lzo select 13,null,null,null,null,null,null,null,null,null,null,null,null,null;
 insert into hudi_parquet_lzo select 14,null,null,null,null,null,12345678901234567890.123456789012345678,null,null,'',null,null,null,null;

create hudi external table:

 create external table ex_hudi_tbl0 (
 uuid int
 ,col_boolean boolean
 ,col_int int
 ,col_long bigint
 ,col_float float
 ,col_double double
 ,col_decimal decimal(38,18)
 ,col_date date
 ,col_string string
 ,col_binary varchar(200))
 ENGINE=hudi
 properties (
 "resource" = "hudi_emr_tn",
 "table" = "hudi_parquet_lzo",
 "database" = "hudi_db");

select from hudi external table:

select col_boolean,col_int,col_long,col_float,col_double,col_decimal,col_date,col_string,col_binary from ex_hudi_tbl0 order by col_int limit 5
execute sql error and messages: (1064, 'unknown compression type(11)'), and time is: 2022-03-29 19:52:41

Expected behavior (Required)
lzo compression surported
Real behavior (Required)
unknown compression type(11)
StarRocks version (Required)

You can get the StarRocks version by executing SQL select current_version()

mysql> select current_version();
+------------------------+
| current_version()      |
+------------------------+
| QA_TEST_MASTER 61b1f1f |
+------------------------+
1 row in set (0.09 sec)

i fill fix it

currently, the lzo compression type is not supported by starrocks so hudi is, but we can add this feature to the future plan


Update component import guidelines

The component package starting page offers the possibility to import components individually like so:
import { defineCustomElement as definePostIcon } from '@swisspost/design-system-components/dist/components/post-icon';
definePostIcon();

However this does not work in the case of nested components. Importing the tabs like so will give an error:
import { defineCustomElement as definePostTabHeader } from '@swisspost/design-system-components/dist/components/post-tab-header';
import { defineCustomElement as definePostTabPanel } from '@swisspost/design-system-components/dist/components/post-tab-panel';
import { defineCustomElement as definePostTabs } from '@swisspost/design-system-components/dist/components/post-tabs';
definePostTabHeader();
definePostTabPanel();
definePostTabs();

TypeError: tab.componentOnReady is not a function
_ref post-tabs2.js:73

### Tasks
- [ ] Check if we can wait for tabs to be defined before calling the onReady function
- [ ] https://github.com/swisspost/design-system/issues/3361

It appears that the componentOnReady method is not available with the "dist-custom-elements" output target, we have to simulate it in this case.


:wrench: Issue #1937: Interact with phonegap-plugin-push

This change works around an incompatibility with Play Services and phonegap-plugin-push. Folks can still over-ride the Play Services version using a preference/variable but if you leave it as + that causes issues with the Google Services Gradle Plugin.
Sorry about the formatting changes. A combination of Prettier, eslint, VS Code auto styling.

Declined. Once I specified  the version number of Google Play services in the past, everyone argued to me about that.
And the developer of the phonegap-plugin-push should update the code with the latest version.

@wf9a5m75 I am the maintainer of phonegap-plugin-push and I'd gladly update the version to whatever you think is most up to date.
The change I am suggesting is enabling users to set the version of Play Services via an environment variable they can set. So if they do:
cordova plugin add cordova-plugin-googlemaps --variable API_KEY_FOR_ANDROID=xxxxx --variable API_KEY_FOR_IOS=xxxxx

it will use the default Play Services value, 11.0.1 in this case, or they can add the plugin using:
cordova plugin add cordova-plugin-googlemaps --variable API_KEY_FOR_ANDROID=xxxxx --variable API_KEY_FOR_IOS=xxxxx --variable PLAY_SERVICES_VERSION=99.9.999

and of course they can update the plugin tag in config.xml as well:
    <plugin name="cordova-plugin-googlemaps" spec="../plugins/cordova-plugin-googlemaps">
        <variable name="API_KEY_FOR_ANDROID" value="xxxxx" />
        <variable name="API_KEY_FOR_IOS" value="xxxxx" />
        <variable name="LOCATION_WHEN_IN_USE_DESCRIPTION" value="Show your location on the map" />
        <variable name="LOCATION_ALWAYS_USAGE_DESCRIPTION" value="Trace your location on the map" />
        <variable name="PLAY_SERVICES_VERSION" value="11.0.1" />
    </plugin>

@wf9a5m75 this plugin has issues with both, cordova-plugin-fcm and cordova-plugin-push. Build fails in both scenarios. @macdonst 's fork is working fine for me.


Frontend assessment 1

Metric
Value

alert_status
:white_check_mark: OK

bugs
3

code_smells
0

reliability_rating
C (at least 1 Major Bug)Note: A being best, E being worst

security_rating
A (0 Vulnerabilities)Note: A being best, E being worst

vulnerabilities
0

Don't push the commented code

Always add proper commit messages
Follow this message syntax <branch_namev : "Commit message (a bit descriptive)">
And if too many commits are there then squash them


Added support for ancient ES3 browsers

Added support for ancient ES3 browsers, like IE6, IE7, IE8 without breaking compatibility with modern browsers. The build output after minifiers will depend on minifier configuration. For example, uglifyjs will keep x['default'] if { ie8: true } is set and will convert back to x.default if not set. This is a safe change that will add ES3 support for free without impact on modern browsers.
Fixes #364

Hi, babel got recently merged full es3 support as a part of preset-es3 (https://github.com/babel/babel/pull/9781). It could be really nice to have rollup to also support es3 and the change needed for commonjs is pretty small. Let me know if I can do anything else to move forward with this PR. Thank you.

Hi @sormy,
I will not make any promises for Rollup itself supporting ES3 any time soon—not too long ago, we removed legacy mode as it was basically too hard to maintain without proper ES3 environment tests and there is a lot of different auto-generated code that will not work in such an environment. Nevertheless, I could see us accomodate this specific change as long as this is not seen as a promise to become fully ES3 compliant.


June 27, 2022 Community Meeting

Please add a topic in this thread and add a link to the GitHub issue associated with the topic.
Please make sure you give folks enough time to review/discuss the topic offline on GitHub before coming into the meeting
(optional) Paste the image of an animal 😸

Community: switch meetings from BlueJeans to Zoom?

/close


Add support for MDX comment syntax

What version of textlint are you using?
v14.0.3
What file type (Markdown, plain text, etc.) are you using?
MDX via @textlint/textlint-plugin-markdown
What did you do? Please include the actual source code causing the issue.
The comment syntax in Markdown is the same as in HTML, but MDX does not support this comment syntax and instead uses {/* */}.
@textlint/textlint-plugin-markdown does not recognize this as a comment, and to treat it as a comment syntax, must do as follows:
{/*  */}

What did you expect to happen?
I expect the MDX comment syntax to be supported and that the range enclosed by the MDX comment syntax would not be subject to linting.
What actually happened? Please include the actual, raw output from textlint.
The range enclosed by the MDX comment syntax is subject to linting.

I think the best way to make a textlint-plugin-mdx.
Actually, MDX and Markdown are not completely compatible.

https://github.com/micromark/mdx-state-machine#72-deviations-from-markdown

textlint-plugin-markdown treat
{/* .... */}
 as HTML fragments.
https://textlint.github.io/astexplorer/#/snippet/woXCqHBhcnNlcklEwrh0ZXh0bGludDptxINrZG93bi10by1hc3TCqMSFdHTEkGdzwoHEisSMxI7EkMSSxJRyxJbEmMSaxJzEnsSgw4DCqHbEhnNpb27EqMSqxI3Ej8SRxJPElcSXxJnEm8SdxJ90wqYxNC4wLjPCqGZpbGVuYW1lwrBzb3VyY2UudW5kZcWUbmVkwqRjb8Wmwr88ZGl2PgogIHsvKiA8IS0tIMW_PiAqL30KPC_FssW0}
textlint-filter-rule-comments extract HTML comment from the fragment and filter it.
https://github.com/textlint/textlint-filter-rule-comments/blob/755c4402cc4995cd7ca37764c138840811d53515/src/textlint-filter-rule-comments.js#L40-L55
If textlint-plugin-mdx parse
{/* .... */}
 as <Comment />, textlint-filter-rule-comments just works without workaround.
It's hard to make a plugin for MDX, so there may be a better way.

I think it is possible to improve the regexp here.
https://github.com/textlint/textlint-filter-rule-comments/blob/755c4402cc4995cd7ca37764c138840811d53515/src/parse-comment.js#L3
However, it is necessary to verify a little whether there is false-negative.
(I don't think many users will write such comments {/* textlint-disable */} unexpectlly, There is no need to worry.)
Actually, {/* */} is not a comment in HTML, so I don't know if we should change this regexp.

Probabely, https://github.com/3w36zj6/textlint-filter-rule-comments/commit/45c4e5b37a36bb59f84e8fd73f8d790268cda698 reuse g ann it is something wrong.
Maybe, we can create a RegeExp dynamically like this.
            const regExp = new RegExp([
                String.raw`<!--\s*${enablingComment}*(?:.|\s)*?-->`,
                String.raw`<!--\s*${disablingComment}*(?:.|\s)*?-->`,
                String.raw`{/\*\s*${enablingComment}*(?:.|\s)*?\*/}`,
                String.raw`{/\*\s*${disablingComment}*(?:.|\s)*?\*/}`
            ].join("|"), "g");
            const matches = nodeValue.matchAll(regExp);
            const comments = Array.from(matches, m => {
                return m[0].replace(/|{\/\*|\*\/}/g, "")
            });
            comments.forEach(commentValue => {
                if (commentValue.indexOf(enablingComment) !== -1) {
                    const configValue = commentValue.replace(enablingComment, "");
                    statusManager.enableReporting(node, parseRuleIds(configValue));
                } else if (commentValue.indexOf(disablingComment) !== -1) {
                    const configValue = commentValue.replace(disablingComment, "");
                    statusManager.disableReporting(node, parseRuleIds(configValue));
                }
            });

I have reverted the changes to HTML_COMMENT_REGEXP and implemented the suggested changes (https://github.com/3w36zj6/textlint-filter-rule-comments/commit/5ec2076fcd1203b34b5646bfb61cae69e3938815).
All tests in Markdown pass, but in MDX, comments continue to be subject to lint, and rule disabling is not performed.

Thank you for your continuous support. We considered creating mdx-to-ast and textlint-plugin-mdx and conducted the following simple experiments.
First, I added remark-mdx to the Processor.
<EMAIL_ADDRESS>import unified from "unified";
// @ts-ignore
import autolinkLiteral from "mdast-util-gfm-autolink-literal/from-markdown";
// FIXME: Disable auto link literal transforms that break AST node
// https://github.com/remarkjs/remark-gfm/issues/16
// Need to override before import gfm plugin
autolinkLiteral.transforms = [];
// Load plugins
import remarkGfm from "remark-gfm";
+import remarkMdx from "remark-mdx";
import remarkParse from "remark-parse";
import frontmatter from "remark-frontmatter";
import footnotes from "remark-footnotes";
import type { Node } from "unist";

-const remark = unified().use(remarkParse).use(frontmatter, ["yaml"]).use(remarkGfm).use(footnotes, {
+const remark = unified().use(remarkParse).use(frontmatter, ["yaml"]).use(remarkGfm).use(remarkMdx).use(footnotes, {
    inlineNotes: true
});
export const parseMarkdown = (text: string): Node => {
    return remark.parse(text);
};

Next, I added processing for MDX-specific mdast nodes.
<EMAIL_ADDRESS>import { SyntaxMap } from "./mapping/markdown-syntax-map";
import type { TxtDocumentNode } from "@textlint/ast-node-types";
import { ASTNodeTypes } from "@textlint/ast-node-types";
import traverse from "traverse";
import debug0 from "debug";
import { parseMarkdown } from "./parse-markdown";

const debug = debug0("@textlint/markdown-to-ast");

export { ASTNodeTypes as Syntax };

/**
 * parse Markdown text and return ast mapped location info.
 * @param {string} text
 */
export function parse(text: string): TxtDocumentNode {
    // remark-parse's AST does not consider BOM
    // AST's position does not +1 by BOM
    // So, just trim BOM and parse it for `raw` property
    // textlint's SourceCode also take same approach - trim BOM and check the position
    // This means that the loading side need to consider BOM position - for example fs.readFile and text slice script.
    // https://github.com/micromark/micromark/blob/0f19c1ac25964872a160d8b536878b125ddfe393/lib/preprocess.mjs#L29-L31
    const hasBOM = text.charCodeAt(0) === 0xfeff;
    const textWithoutBOM = hasBOM ? text.slice(1) : text;
    const ast = parseMarkdown(textWithoutBOM);
    traverse(ast).forEach(function (node) {
        // eslint-disable-next-line no-invalid-this
+        const isMdx =
+            node &&
+            Object.prototype.hasOwnProperty.call(node, "type") &&
+            /^(mdxJsxFlowElement|mdxJsxTextElement|mdxjsEsm|mdxFlowExpression|mdxTextExpression)$/.test(node.type);
        if (this.notLeaf) {
+            if (isMdx) {
+                if (node.type === "mdxJsxFlowElement") {
+                    node.type = ASTNodeTypes.Html;
+                } else if (node.type === "mdxJsxTextElement") {
+                    node.type = ASTNodeTypes.Paragraph;
+                } else if (
+                    node.type === "mdxjsEsm" ||
+                    node.type === "mdxFlowExpression" ||
+                    node.type === "mdxTextExpression"
+                ) {
+                    if (Object.prototype.hasOwnProperty.call(node, "value") && /^(\/\*(.|\s)*\*\/)$/.test(node.value)) {
+                        node.type = ASTNodeTypes.Comment;
+                        node.value = node.value.replace(/^(\/\*)/, "").replace(/\*\/$/, "");
+                    } else {
+                        node.type = node.type === "mdxTextExpression" ? ASTNodeTypes.Code : ASTNodeTypes.CodeBlock;
+                    }
+                }
+                if (Object.prototype.hasOwnProperty.call(node, "attributes")) {
+                    delete node.attributes;
+                }
+                if (Object.prototype.hasOwnProperty.call(node, "name")) {
+                    delete node.name;
+                }
+                if (Object.prototype.hasOwnProperty.call(node, "data")) {
+                    delete node.data;
+                }
-            if (node.type) {
+            } else if (node.type) {
                const replacedType = SyntaxMap[node.type as keyof typeof SyntaxMap];
                if (!replacedType) {
                    debug(`replacedType : ${replacedType} , node.type: ${node.type}`);
                } else {
                    node.type = replacedType;
                }
            }
            // map `range`, `loc` and `raw` to node
            if (node.position) {
                const position = node.position;
                // line start with 1
                // column start with 0
                const positionCompensated = {
                    start: { line: position.start.line, column: Math.max(position.start.column - 1, 0) },
                    end: { line: position.end.line, column: Math.max(position.end.column - 1, 0) }
                };
                const range = [position.start.offset, position.end.offset] as const;
                node.loc = positionCompensated;
                node.range = range;
                node.raw = textWithoutBOM.slice(range[0], range[1]);
                // Compatible for https://github.com/syntax-tree/unist, but it is hidden
                Object.defineProperty(node, "position", {
                    enumerable: false,
                    configurable: false,
                    writable: false,
                    value: position
                });
            }
        }
    });
    return ast as TxtDocumentNode;
}

This approach will likely succeed. However, the latest remark-mdx only supports ESM, so I believe updates to configuration files and dependencies will be necessary before proceeding.
(I updated the configuration files and dependencies and confirmed operation on local environment, I could not figure out how to make both the TypeScript transpile and the test with mocha succeed)

The textlint-plugin-mdx has been transferred to the textlint organization, but I forgot to close this issue.


Wrongly add domain to the exceptions due to the incorrect https detection

VERBOSE, AdguardSvc, 13, 30.01.2019 14:35:14.635, Adguard.Core.Logging.CoreLoggerAdapter | AGProxySession: [id=1000040] Set operating mode to DETECTION
VERBOSE, AdguardSvc, 13, 30.01.2019 14:35:14.635, Adguard.Core.Logging.CoreLoggerAdapter | AGNFHandler: tcpSend: [id=652] returned
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.635, Adguard.Core.Logging.CoreLoggerAdapter | AGNFHandler: wantRead: [id=652] indicating deferred read len=201
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.739, Adguard.Core.Logging.CoreLoggerAdapter | PF: Certificate verification for github.com returned with status 0 ()
VERBOSE, AdguardSvc, 31, 30.01.2019 14:35:14.746, Adguard.Core.Logging.CoreLoggerAdapter | AGNFHandler: tcpSend: [id=652] returned
VERBOSE, AdguardSvc, 31, 30.01.2019 14:35:14.746, Adguard.Core.Logging.CoreLoggerAdapter | AGNFHandler: tcpSend: [id=652] returned
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.747, Adguard.Core.Logging.CoreLoggerAdapter | AGSslDomainFilter: [id=1000040] TLS connection is not HTTPS, adding 'C:\Program Files (x86)\Google\Chrome\Application\chrome.exe#<IP_ADDRESS>:443#github.com' to exceptions
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.747, Adguard.Core.Logging.CoreLoggerAdapter | AGProxySession: [id=1000040-ssl] Set operating mode to PASS
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.747, Adguard.Core.Logging.CoreLoggerAdapter | AGProxySession: [id=1000040-pass-ssl] Can't detect protocol, switching to pass mode
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.747, Adguard.Core.Logging.CoreLoggerAdapter | AGProxySession: [id=1000040-pass-ssl] Set operating mode to PASS
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.747, Adguard.Core.Logging.CoreLoggerAdapter | AGProxySession: [id=1000040-pass-ssl] Shutting down session (from=430)
VERBOSE, AdguardSvc, 41, 30.01.2019 14:35:14.747, Adguard.Core.Logging.CoreLoggerAdapter | AGProxySession: [id=1000040-pass-ssl] Destroying session

core/pull-requests/1009

Testing instructions:

enable EV certificates filtering
go to https://github.com/ or any other site with EV certificate (like meduza.io)
site is expected to be filtered (certificate is changed to Adguard's one, assistant is shown if enabled)


Deprecate ready/readyPromise for terrainProviders

Part of https://github.com/CesiumGS/cesium/issues/10909
Since we're getting rid of the readyPromise pattern, it's preferable to roll out the deprecations across the API as part of one release in order to reduce noise. I've created a staging branch to target some smaller PRs like this into.
This PR depreciates the readyPromise pattern for terrain providers. I set the deprecation period for two releases from the next one (1.104) since these are fairly high-tough areas of the API. A few other API changes consequently fall out of this deprecation, notably:

ready and readyPromise have been deprecated, but remain working until they are completely removed.
Various properties no longer throw an exception since there's no need to wait for ready
Where applicable, url or the relevant resource-related options have been deprecated from constructors. In their place, there are new fromUrl functions, which are asynchronous functions that resolve to a new instance of the provider after successfully requesting the required resources. This function call throws any relevant errors rather than logging it to the console or raising an error event.
Viewer and CesiumWidget parameters: These will now need to take promises which resolve to the terrainProvider if we'd like to keep the same setup flow
createWorldTerrain will need to be deprecated, as it can no longer work synchronously. I went with createWorldTerrainAsync but I'm open to naming suggestions.

Some finer points of the implementation here include:

Generally, a move towards using async/await syntax for flatter and more readable code
Adds a new async version of the toThrowDeveloperError matcher for better async testing flow
Making use of a builder pattern (encapsulated within the modules, not part of the public API) in order to manage a more complex object construction
I don't think the baseLayerPicker and Cesium Viewer app changes  are the cleanest they can be, though it's probably best to address that once the imagery provider version of these changes are in.
It looks like in order for typescript to pass, we cannot remove the url option from the the constructor options typedef without throwing an error. Unless there is a workaround I don't know of, we'll need to note to remember to remove the property when the depreciation warnings are removed.

Finally, as a consequence of these changes, we'll definitely need to do a pass through website tutorials and ensure we're using the updated APIs. I've made the Sandcastle changes here already.
TODO:

[x] Cleanup typescript errors
[x] Make sure sandcastles with dropdowns have selected their first item
[x] scene.pickPosition seems wildly off in http://localhost:8080/Apps/Sandcastle/index.html?src=Drawing on Terrain.html&label=All
[x] I3S Error http://localhost:8080/Apps/Sandcastle/index.html?src=I3S IntegratedMesh Layer.html&label=All
[x] Put back removed unit tests

Outside of scope for this PR:

[ ] Pass over async code for Sandcastles (and do we want to make the invoked wrapper function async). In particular, do a pass for race conditions when doing things like switching providers.
[ ] depthTestAgainstTerrain setting via BaseLayerPicker? https://github.com/CesiumGS/cesium/pull/11033#issuecomment-1400538785
[ ] Finalize deprecation versions and open issues
[ ] fromUrl / Resource naming
[ ] Open forum thread to discuss API changes

Thanks for the pull request @ggetz!

:heavy_check_mark: Signed CLA found.
:grey_question: Unit tests were not updated.

Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

[ ] Cesium Viewer works.
[ ] Works in 2D/CV.

@mramato Would you be able to give this a pass?

@ggetz Will do. Thanks!

Adds a new async version of the toThrowDeveloperError matcher for better async testing flow

I haven't looked at the code yet, but modern jasmine allows you to do stuff like:
expectAsync(promimse).toBeRejectedWithError(DeveloperError);
or even
expectAsync(promimse).toBeRejectedWithError(DeveloperError, "Exact error message");
or
expectAsync(promimse).toBeRejectedWithError(DeveloperError, /regex_message_check/);
I feel that conventions in Cesium like toThrowDeveloperError are data and custom for no longer valid reasons. This isn't the PR to start changing that, but it would be nice to update the code base to start to move towards modern jasmine.

It looks like in order for typescript to pass, we cannot remove the url option from the the constructor options typedef without throwing an error. Unless there is a workaround I don't know of, we'll need to note to remember to remove the property when the depreciation warnings are removed.

Can you clarify what you mean by this. Are you talking about leaving the deprecated url property in the constructor options JSDoc so that TypeScript still includes it?

I don't have a better suggestion, but did we consider the fact that CesiumTerrainProvider.fromUrl is kind of an odd name since it also takes a Resource instance? I ask because it doesn't seem super intuitive to have code like:
const terrain = await CesiumTerrainProvider.fromUrl(IonResource.fromAssetId(1234));

Perhaps this should continue to work and in some future PR we can add CesiumTerrainProvider.fromIonAssetId as a helper function that would simplify the above and make the code more self documenting?

http://localhost:8080/Apps/Sandcastle/index.html?src=Procedural Terrain.html is broken as well (it doesn't load until you pick something from the combo box. i.e. it's flat at the start instead of Noise being rendered.

@ggetz That's all I have for my pass. This looks like it will be a great change and lead to a much cleaner API. (In addition to fixing the race conditions and unit testing issues)

It looks like in order for typescript to pass, we cannot remove the url option from the the constructor options typedef without throwing an error. Unless there is a workaround I don't know of, we'll need to note to remember to remove the property when the depreciation warnings are removed.
Can you clarify what you mean by this. Are you talking about leaving the deprecated url property in the constructor options JSDoc so that TypeScript still includes it?

We commonly break out the options object for constructors into there own typedef that we defined with JSDoc, ie @typedef {Object} CesiumTerrainProvider.ConstructorOptions, particularly if they are re-used, such as they are in this PR across the constructors and the new fromUrl functions. Attempting to remove the url property from the typedef causes our typescript generation step to throw errors since options.url is still being accessed in the deprecated code paths.
To be clear that this property is deprecated now, I have instead marked the property as optional and added a short note that the property is deprecated. I can also look deeper to see if there is a way to avoid the error if it jumps out as unusual.

I feel that conventions in Cesium like toThrowDeveloperError are data and custom for no longer valid reasons. This isn't the PR to start changing that, but it would be nice to update the code base to start to move towards modern jasmine.

Generally, this PR does start moving towards this convention. The main motivation for a custom toThrowDeveloper matcher now is that it becomes a noop in the release tests. Otherwise in the case of RuntimeErrors, we use expectAsync(promise).toBeRejectedWithError(RuntimeError, "Exact error message");

but don't we normally leave old deprecated tests in to ensure the old functionality still works?

My mistake; I did verify that the old tests were working before changing them to the new APIs. Easy to put them back in.

@mramato

http://localhost:8080/Apps/Sandcastle/index.html?src=I3S Feature Picking.html seems to be broken?

How so? Running this locally looks exactly the same as https://sandcastle.cesium.com/?src=3D Tiles Feature Picking.html on my machine.

It wasn't loading the tileset at all for me. I'll try again and confirm.

Updated to move all the asynchronous calls to the top level for the app level to await and handle errors. As a consequence, a lot of the Sandcastle code is now surrounded by an async function block. I added try/catch to the more minimal examples concentrating on terrain providers. I'd expect more Sandcastle changes to come as we finish up the rest of the ready promise deprecations throughout the codebase, and then we can do a final pass.

I'd expect more Sandcastle changes to come as we finish up the rest of the ready promise deprecations throughout the codebase, and then we can do a final pass.

I haven't looked yet, but from what I remember with Sandcastle's bucket system it should actually be easy to just wrap the entire Sandcastle demo code in an async block behind the scenes so that top-level await works there. Could make for a good follow-up PR that is generally useful and cleans up the examples.

I noticed that https://github.com/CesiumGS/cesium/issues/4368 was popping up, and that I had to add globe.depthTestAgainstTerrain = true in the Drawing on Terrain Sandcastle to work around the issue. It turns out that depthTestAgainstTerrain but being set to true from within the baseLayerPicker widget when a terrain provider was passed to Viewer (https://github.com/CesiumGS/cesium/issues/9985). This PR is probably not the place to address it, but setting this value from within the widget seems very counter intuitive, especially if a bug as obvious as https://github.com/CesiumGS/cesium/issues/4368 ends up showing up.

I think I've addressed everything so far that is in scope for this intermittent PR. @jjhembd Could you please do a pass on reviewing this PR?
Since this has already brought up a few questions that we'll want to address before merging the final staging branch into main, I've added an "outside of scope for this PR" section, which I'll transfer to the staging branch PR after this is merged so we can keep track.

I apologize in advance for this, because it's going to be pretty annoying, especially this late... but, I don't really think this is the right direction for the API...
With this change, imagery providers, terrain providers, and other things can only be constructed asynchronously (ignoring unlikely scenarios where the caller happens to have all the data needed to construct it synchronously). And there's a fromUrl factory function to do the async construction.
The problem is that this is super hard for users to deal with. Particularly users who don't understand promises or async/await very deeply, which is many (most?) of them. And even for those who do, the asynchronicity is a huge source of complexity and therefore bugs.
If the only (practical) way to construct an ImageryProvider is with a function that returns a promise, then we only have a few patterns that can be used to add that imagery provider to the globe:
Wait for the imagery provider, then add it to the globe
Something like:
const imageryProvider = await WebMapServiceImageryProvider.fromUrl(...);
globe.imageryProviders.add(imageryProvider);

Which looks innocent enough, but now this entire operation is asynchronous. The user just clicked a button to add this WMS layer, let's say. Do we make them look at an hourglass while we complete this async operation? If not, what if the user changes their mind and wants to remove it before the async operation has completed? There are ways to deal with this, but it is super fiddly and easy to get wrong.
Allow a Promise<ImageryProvider> to be added directly
Something like:
globe.imageryProviders.add(WebMapServiceImageryProvider.fromUrl(...));

But now the imageryProviders collection has a mix of real imagery providers and promises for imagery providers. In TypeScript this is awkward in the type system. Without TypeScript its a recipe for runtime errors when someone writes code without remembering that the elements can sometimes be promises.
Make creation of the entire globe async
We can wait for this promise to complete constructing the viewer. The example in the CesiumTerrainProvider docs in this branch suggests this pattern for use with a terrain provder:
const viewer = new Cesium.Viewer("cesiumContainer", {
  terrainProvider: await Cesium.CesiumTerrainProvider.fromUrl(
    Cesium.IonResource.fromAssetId(3956), {
      requestVertexNormals: true
    });

It's a bit subtle at a glance, but in this pattern the Cesium.Viewer won't be constructed until the terrain provider is constructed asynchronously. Maybe not a huge problem for a terrain provider, but for things that are meant to be layered on top, like overlay imagery layers and some 3D Tilesets, it's a step backwards versus showing the globe more quickly and then adding other data when it's available.

Ok, so what's the alternative? I think the right solution is the pattern we eventually landed on in cesium-native: separate the factory from the asynchronously-created provider. In cesium-native, the "imagery provider" is called RasterOverlay. Raster overlays can be constructed synchronously, but they can't provide tiles directly. Instead, someone who wants tiles (usually the rest of cesium-native itself) calls createTileProvider on the raster overlay. This method returns a promise for a RasterOverlayTileProvider.
In some sense this isn't so different from what's implemented here. It's just a factory class that can asynchronously create a tile provider, rather than a factory function that can asynchronously create a tile provider. But in another sense, it makes all the difference. It means that users can easily create new imagery providers, terrain provider, tilesets, etc. and add them to the globe immediately. And when they inspect the appropriate properties on the globe or whatever, they get the same instances back.
Errors are handled by whoever calls createTileProvider, which is usually code written by us so we can deal with the complexity of the asynchronicity. Errors can be surfaced to users as events. Events can also be used to inform users when async operations have completed.
IMO this will be much easier for our users to deal with than async factory functions.

Thanks, @kring. Not that me and you are deeply involved lately, but we built CesiumJS on "disagree and commit" so it's never too late to speak up. :smile: Obviously everyone wants the best approach to win.
Just to keep our eye on the prize. Ultimately the problem here is that CesiumJS has a bunch of unknowable/unawaitable asynchronicity happening under the hood in the "middle" of the stack. That's what leads to problems in #10909 and the fact that Cesium CI just randomly fails these days due to unhandled promises.
This PR (based on conversations I had with Gabby) pushes all of the async code to the top of the stack. Kevin's idea seems to be to go to the complete opposite direction and push it further into the engine and use events to bubble errors back up to the top.
I think both concepts are equally valid and can both be used to achieve the intended goal, making sure all async operations are "knowable" and "awaitable". However I am biased towards the approach this PR originally took.

The problem is that this is super hard for users to deal with. Particularly users who don't understand promises or async/await very deeply, which is many (most?) of them. And even for those who do, the asynchronicity is a huge source of complexity and therefore bugs.

I would want to see some data to back up this assertion. This may have been true 10 years ago, but promises/async/await are now at the core of JS development. Everyone uses them, everyone knows enough about them for them not to be some weird new concept like when we introduced them into CesiumJS' code base. They are bog standard JS.

The user just clicked a button to add this WMS layer, let's say. Do we make them look at an hourglass while we complete this async operation?

Browser apps are async by nature since a lot of operations are reaching back to the server. As an extreme example, every single letter you type into Google Docs makes a network request. Almost all JS apps and frameworks have opinionated ways for dealing with this. Old school hourglasses like you suggest, optimistic + rollback, or some other form of async error handling. Ultimately my point is that async in a web app is not the big lift you seem to be painting it as.

Errors are handled by whoever calls createTileProvider

What is multiple people need to tile provider and it's not clear who will get to it first? Do people create multiple providers rather than sharing a single one? What about when a user is the first one that needs the tile provider? It sounds like now you're pushing the async handling on them anyway.

Errors can be surfaced to users as events.

To me, this seems like it would be more confusing to most users than just using async/await. Ultimately what your suggesting sounds very C++/thick client approach to me. Which works great in those environments but would be less expected in JavaScript.
Also, Kevin's approach sounds like a bigger refactor to me? (though maybe it's not).
In my experience taking other JS apps with this problem and pushing things up to the top of the stack always ended up being a big improvement. The more potential async code you can remove each frame, the better.
Ultimately, it's up to @ggetz and the CesiumJS team to make this decision, as long as apps built on Cesium ensure all async operations are knowable/awaitable, it will be an improvement over what we have now and I'll be happy.

I'll start with the thought that I'm clearly biased towards the solution which is already in motion... 😆 I'm generally in favor of moving the up the stack to let the user of the library decide what is right for their app.

The problem is that this is super hard for users to deal with. Particularly users who don't understand promises or async/await very deeply, which is many (most?) of them. And even for those who do, the asynchronicity is a huge source of complexity and therefore bugs.

If we do proceed with exposing the async functionality, I do think the onus is on us to make sure we are properly instructing devs how to use the async functionality. So updating our examples to make it clear how to construct any providers, properly handling errors, etc.
From a maintainability standpoint, I don't think we've been great about handling all the errors that may pop up throughout the stack, which has lead to bugs, especially intermittent ones. As https://github.com/CesiumGS/cesium/issues/10909 points out, this new structure makes thing much more testable, especially since asynchronous code has become a part of testing libraries like jasmine.

The user just clicked a button to add this WMS layer, let's say. Do we make them look at an hourglass while we complete this async operation? If not, what if the user changes their mind and wants to remove it before the async operation has completed? There are ways to deal with this, but it is super fiddly and easy to get wrong.

I do think handling asynchronous state management is a common paradigm in web development. For example in some of our other apps we've been working on at Cesium, we've been working with Redux where there are helper functions which abstract the recommend approach for handling async requests.

separate the factory from the asynchronously-created provider

It may be worth pointing out (and I may be totally off base with this since I'm not very involved with the C++ world), I do think a C++ and JS implementation will look different when it comes to handling asynchronous code. Promises and async code are a standard part of modern JS, and while std::future exists, I don't think its used as ubiquitously as promises in JS.
If I correctly understand the RasterOverlay and RasterOverlayTileProvider pattern, we would basically want an additional class that lives on top of the existing provider that handles errors and dispatches events, correct? I don't think that precludes the approach here, but it would require that we add that top level RasterOverlay abstraction before shipping this update.
Ultimately, I'm leaning on the side of staying leaner and foregoing that RasterOverlay abstraction, as it will give the user better flexibility in their own apps, and will give us less API area to maintain.
Perhaps in the future, as declarative paradigms are becoming more common in the JS world, we may re-introduce that level of abstraction in the to provide a better interface for event-driven apps, but I'm not sure that's the right choice for the library as it is today.

@kring, I agree that the last pattern ("Make creation of the entire globe async") is often not a good one, for the reasons you mentioned:

for things that are meant to be layered on top, like overlay imagery layers and some 3D Tilesets, it's a step backwards versus showing the globe more quickly and then adding other data when it's available.

However, this can still be addressed with the following pattern, lifted from the "Terrain Performance" Sandcastle: (it's another version of your first pattern, "Wait... then add it to the globe".)
        const viewer = new Cesium.Viewer("cesiumContainer", {});

        (async () => {
          try {
            viewer.terrainProvider = await Cesium.createWorldTerrainAsync();
          } catch (error) {
            console.log(error);
          }
        })();

        // ... do more stuff with viewer, while the terrainProvider is still loading ...

Code that depends on terrainProvider actually being loaded, now has to go inside the async function. Previously, it had to go inside terrainProvider.readyPromise.then(...).
From the app developer's point of view, I think either approach requires a similar level of async understanding to code correctly. But the new pattern better handles coding mistakes. The old approach depended on things like this internally:
  if (!this._ready) {
    throw new DeveloperError(
      "requestTileGeometry must not be called before the terrain provider is ready."
    );
  }

and on the hope that we had these errors in all the right places, and they were documented, and the user read all the documentation...

@ggetz, sorry if I missed something, but are we planning to clean up more of the .readyPromise usage in the specs in this PR? I understand we still want to test the deprecated methods, but there are unrelated tests like this one in CesiumTerrainProviderSpec
  it("returns reasonable geometric error for various levels", function () {
    returnQuantizedMeshTileJson();

    const provider = new CesiumTerrainProvider({
      url: "made/up/url",
    });

    return provider.readyPromise.then(function () {
      expect(provider.getLevelMaximumGeometricError(0)).toBeGreaterThan(0.0);

(async () => {

I'll also point out that in modern JS, top-level await means no weird wrapping of async stuff.

no weird wrapping of async stuff.

It does look weird, and made me question the new API at first. But on the other hand, without wrapping it, what if the terrain provider hangs? How could we make sure the rest of the app is still usable? The wrapping lets the rest of the function execute in the meantime.

I understand we still want to test the deprecated methods, but there are unrelated tests like this one in CesiumTerrainProviderSpec

I swung the opposite way to prove that all the correct properties were still being set on created such that these tests continued to work. But based the coding guide (emphasis mine) they should get switched over.

Remove all use of the deprecated API inside Cesium except for unit tests that specifically test the deprecated API

The test will certainly be briefer with the new API, so I'm not against switching it. I'll update.

I would want to see some data to back up this assertion. This may have been true 10 years ago, but promises/async/await are now at the core of JS development. Everyone uses them, everyone knows enough about them for them not to be some weird new concept like when we introduced them into CesiumJS' code base. They are bog standard JS.

I used to interview JS developers with a question based around promises. Use async/await if you want, totally open book. I'd even answer any questions you had about promises, their API, how they work, whatever. It was very very rare that someone did well on it. Even people that had experience using async/await and promises seemed to have a very shaky understanding of what they actually did. I'm sure this situation is improving, but the fact remains that this stuff is hard and most of the time you don't need to understand it very deeply to be a competent JS developer.

What is multiple people need to tile provider and it's not clear who will get to it first? Do people create multiple providers rather than sharing a single one? What about when a user is the first one that needs the tile provider? It sounds like now you're pushing the async handling on them anyway.

These questions seem really academic. In virtually all Cesium applications, the only client of tile providers is Cesium itself. Users create them to tell Cesium what to show on the globe, but never "use" them otherwise.
If a user does want a tile provider themselves for some reason, then yes, they need to deal with the asynchronicity. That's a far more advanced scenario than "I want to show my WMS layer on this map."

From a maintainability standpoint, I don't think we've been great about handling all the errors that may pop up throughout the stack, which has lead to bugs, especially intermittent ones. As https://github.com/CesiumGS/cesium/issues/10909 points out, this new structure makes thing much more testable, especially since asynchronous code has become a part of testing libraries like jasmine.

I can see your point about it making Cesium easier to test, but I think it's doing that at the cost of being harder to use.
And it doesn't do anything to guarantee errors are handled. Before, there was a readyPromise available as a property, and we forgot to use it. Now the promise is returned by value, but if we don't handle possible promise rejection, we're no better off. In my proposal, only Cesium has to handle the promise rejection in almost all scenarios. Users don't need to worry about it.

It may be worth pointing out (and I may be totally off base with this since I'm not very involved with the C++ world), I do think a C++ and JS implementation will look different when it comes to handling asynchronous code. Promises and async code are a standard part of modern JS, and while std::future exists, I don't think its used as ubiquitously as promises in JS.

This general idea, "C++ is different", was mentioned a few times. But cesium-native's async system is very much modeled on javascript promises. I don't think this is a valid way to dismiss the proposal.

If I correctly understand the RasterOverlay and RasterOverlayTileProvider pattern, we would basically want an additional class that lives on top of the existing provider that handles errors and dispatches events, correct?

No, the RasterOverlay doesn't handle errors and dispatch events. The client does, meaning the ImageryLayerCollection or globe or similar. The RasterOverlay is a configurable factory for a tile provider. Pretty similar to the TerrainProviderBuilder actually.
I think @jjhembd's example illustrates my overall point really well, so let's walk through it... He proposed:
const viewer = new Cesium.Viewer("cesiumContainer", {});

(async () => {
  try {
    viewer.terrainProvider = await Cesium.createWorldTerrainAsync();
  } catch (error) {
    console.log(error);
  }
})();

// ... do more stuff with viewer, while the terrainProvider is still loading ...

Let's shift this to a real-world example. I have an application that's showing a globe (terrain) and a base layer. And my app allows users to enable a WMS overlay on top of that. So my viewer already exists, and I'm writing a function to add the WMS layer, following Jeshurun's pattern and meant to be called in a click handler or whatever. It looks like this:
function addFancyOverlayToTheGlobe(viewer) {
  (async () => {
    try {
      viewer.imagery.add(await WebMapServiceImageryProvider.fromUrl(...));
    } catch (error) {
      console.log(error);
    }
  })();
}

Of course I could make this an async function itself and return a promise, but then the client needs to be willing to deal with it. If this is a simple click handler or similar, the buck needs to stop here.
Now when the user clicks this button, we start the process of adding the WMS overlay, and sometime later it actually happens. The application needs to keep track of this "something is being added to the globe" state in order to show the user that something is happening, because Cesium doesn't know about it; it's hidden in a continuation. If you ask Cesium which WMS layers are on the globe, this one won't show up yet.
Now what if this isn't an "add button", but a checkbox. The user can toggle the overlay on and off. Super common thing, right? We'd need some way of stopping this async operation later. Maybe something like:
function addFancyOverlayToTheGlobe(viewer) {
  (async () => {
    try {
      const provider = await WebMapServiceImageryProvider.fromUrl(...);
      if (overlayIsStillShown()) {
        viewer.imagery.add(overlay);
      }
    } catch (error) {
      console.log(error);
    }
  })();
}

Hmm ok, now what if the user toggles it on, off, then on again before the fromUrl completes?
What if the user does something that causes the viewer to be destroyed more the fromUrl completes?
These are all tractable problems, but they're the sort of thing developers are extremely likely not to think about. And this is a typical, bog standard, simplest sort of thing you can go with CesiumJS scenario here!
The fundamental problem is that users don't (and shouldn't) view imagery provider creation as an async operation that will complete later. They view it as a description to Cesium of what should be shown on their globe. Add an imagery provider to the globe, and it should appear (eventually). Remove the imagery provider, and it should disappear.
And this is what the RasterOverlay system achieves. But let's stick to CesiumJS's terminology to try to be less confusing. Here's what I'm proposing users should be able to do:
// note: Imagery, not ImageryProvider
// This is lightweight and doesn't do any network requests
const fancyOverlay = new WebMapServiceImagery(...);

function addFancyOverlayToTheGlobe(viewer) {
  viewer.imagery.add(fancyOverlay);
}

function removeFancyOverlayFromTheGlobe(viewer) {
  viewer.imagery.remove(fancyOverlay);
}

All of the async creation of a tile provider, as well as the async requesting of tiles, is handled by the Viewer / Globe / ImageryLayerCollection.
If users need to be notified of imagery provider errors, beyond just showing the error to the user (which should be the default), they can subscribe to an event. A simple API can also be used to query the status of the imagery: is it still doing the initial load? are tile requests in progress? did something go wrong?
Writing tests is just as easy as it is in this PR. Call createTileProvider directly (if you need it), and handle the returned promise's resolution or rejection. But the experience for a typical user of CesiumJS is greatly improved.

I guess the fundamental question here is whether CesiumJS should be what's tracking the state of your application.

I disagree. The fundamental question here is whether this change makes CesiumJS harder to use.

IMO no– CesiumJS already is a huge library that does a lot, plus there are plenty of other solutions that developers may want to use here instead.

I hope I haven't given the impression that I think CesiumJS should be responsible for all of an application's state. That seems a bit of a strawman.
But CesiumJS is certainly not some immediate-mode application where the developer is responsible for managing all loading and then dispatching things to be rendered each frame. It's extremely stateful. Comparing terrain providers, imagery providers, 3D Tilesets, and even geometries to simple "load once" async resources in other engines is not really fair.
But I feel like I'm shouting into the wind here, so how about this... If you want hard evidence that these changes make CesiumJS too hard to use correctly, you need look no further than the Sandcastle code in this branch, where we can easily find an example of it being used incorrectly in exactly the way I'm worried about!
Let's look at the Terrain Sandcastle example. First I'd like to point out that the viewer construction code went from this:
const worldTerrain = Cesium.createWorldTerrain({
  requestWaterMask: true,
  requestVertexNormals: true,
});
const viewer = new Cesium.Viewer("cesiumContainer", {
  terrainProvider: worldTerrain,
});

To this:
const viewer = new Cesium.Viewer("cesiumContainer");
(async () => {
  try {
    viewer.terrainProvider = await Cesium.createWorldTerrainAsync({
      requestWaterMask: true,
      requestVertexNormals: true,
    });
  } catch (error) {
    console.log(error);
  }
})();

Which seems like a huge step back in usability right there. And it's not like it's any safer or handling errors better, either. But let's move on from that and talk about the actual bug.
This Sandcastle example has a dropdown that allows the user to select a terrain provider. The onselect handler for each constructs the appropriate terrain provider, awaits it, and then sets it as the viewer's terrainProvider property. Seems simple enough, right?
Except it has a serious race condition. If I select the VRTheWorldTerrainProvider (which is a slower one from here in Australia), and then very quickly switch to ArcGISTerrainProvider (just because it's the next one up so it's easy to switch to it quickly), the UI will indicate I'm looking at ArcGISTerrainProvider while I'm actually looking at VR The World. I was able to make this happen quite easily within a couple of tries (I confirmed it with the Network tab).
So this is a rather subtle - but potentially very serious! - bug. It didn't happen before this PR. In fact, it would have been quite hard to do it accidentally. Now, with this PR, it's not only extremely easy to do this accidentally (even the world experts in CesiumJS development made this mistake!), but it's also quite difficult to fix. At the very least, the Sandcastle example would get significantly more complicated if we were to fix this.
And this isn't some quirk of Sandcastle. Virtually any CesiumJS app - especially the simpler ones - are likely to run into problems like this.
Now, you don't have to like my solution, but can we at least agree there's a problem?

Which seems like a huge step back in usability right there.

This is a symptom of Sandcastle (which was mentioned up thread as something we should fix) and not a problem elsewhere. Top level await means that the "good" example you posted works in the new API as well, simply by adding await to the call:
const worldTerrain = await Cesium.createWorldTerrainAsync({
  requestWaterMask: true,
  requestVertexNormals: true,
});
const viewer = new Cesium.Viewer("cesiumContainer", {
  terrainProvider: worldTerrain,
});

Also, even without top-level and without this change, it's fairly typical for an application to reach out to a server during initialization, so I don't see how this is much different. Most JS apps already have code dealing with asynchronousness at this level.

I was able to make this happen quite easily within a couple of tries (I confirmed it with the Network tab).

Yes, this is definitely a bug that should be fixed in our widgets. Thanks for bringing it up. This is a branch PR going into the staging branch so there is certainly a list of remaining TODOs that needs to be generated before it goes into main. But the components themselves already have to manage state (like which provider is selected) so I'm not sure why dealing with the async nature suddenly changes everything? As @ggetz mentioned, every modern toolkit out there has async state handling. CesiumJS widgets are definitely not "modern" but I don't see this as a major problem that has technical barriers.

I'm not sure what you're talking about, Matt. This isn't a problem with Sandcastle or "our widgets". Sandcastle.addToolbarMenu is a thin wrapper over a standard HTML select element. It's meant to be a convenience, not an abstraction. Yes, I don't doubt you can framework your way out of this bug, but I'm not really sure how you plan to fix it without introducing some abstractions that would be wildly inappropriate in the context of Sandcastle.
But I'm interested to see what you have in mind; please prove me wrong.
I'm writing too much and I'm not sure it's really being read well (I get it, everyone is super busy, Kevin is super noisy), but a couple of really brief things:

We need to run this change by the community. Probably not with just terrain providers, because no one cares. But once we have imagery providers following this pattern, we definitely need to let the community try it out before we ship it. This may very well be the biggest breaking change in CesiumJS's history; the community deserves to be involved.

I'll try to find some time to prototype the alternate architecture I'm blabbering about, just so you all can see it concretely. IMO it has all the advantages of what's in this PR while not being a breaking change for users (other than users writing their own terrain and imagery providers and that sort of thing), and is in general much easier to use. And it's really not a lot of extra code; very similar to the Builder thing already going on in this PR, actually.

I agree that we should give the community at heads up about this planned change and listen to potential feedback. We'll open a forum thread shortly. This isn't going out with the February 1st release, though I wouldn't like to delay it much longer.
In the meantime, I think we should proceed with getting this PR into a staging branch. From there, we can make further changes such as the imagery providers and have something more concrete to share with the community. @kring We'll still welcome feedback and comments such as your prototype architecture if you end up having the time.
As I mentioned further up in this PR, I don't expect this PR to be the final iteration through Sandcastle code or our own widgets. I think the comment about onSelect and a few other scenarios are valid, but can be handled and discussed further in a later PR.  Also there no need to use a framework to solve these problems. My point was more about how entrenched  the async pattern is is the JS ecosystem.

Thanks @kring for pointing out the issue with the Terrain Sandcastle.
Loading a new Terrain is inherently async, but the current Cesium behavior stubs in a non-functional provider while a new one is loading. On my machine, with the network throttled, the stub makes the globe disappear for a few seconds. I agree this might be less misleading than continuing to show the previous terrain in the meantime. (A non-blocking "Loading" message might be even better, since the user would still be able to use the rest of the app.)
We already have a TODO item to fix these kinds of issues in Sandcastle. And as you mentioned, imagery providers will provide a better use-case to discuss the new API with the community.
I'll merge this in for now, but please continue the discussion as we work on the imagery providers.

As a casual reader of the discussion here, I think the distinction in my mind is that configuration should be synchronous where possible, even when the actual execution is asynchronous.
Top-level await doesn't really improve the situation because you've just made your configuration be blocked by execution, which leads to the performance consequences Kevin alludes to.
"Configuration" in this context refers to the information needed to create a Viewer (and more broadly set up the rest of the application). In this case the information is "I want to use World Terrain (from a given location or whatever)". You don't need any actual data to be loaded from the server to communicate that, generally speaking. So yeah, as Kevin says above, that essentially boils down to a factory. Maybe a bit more if it's going to keep track of created instances, or report information about their state (e.g. failure state).
I admit I don't really have a stake here these days, but at least I can say you're not "shouting into the wind", Kevin.

Well, said, Scott. That's a good way to frame it. Have I mentioned lately that I miss working with you? :)

Loading a new Terrain is inherently async, but the current Cesium behavior stubs in a non-functional provider while a new one is loading. On my machine, with the network throttled, the stub makes the globe disappear for a few seconds. I agree this might be less misleading than continuing to show the previous terrain in the meantime.

This is a really interesting point, it definitely gave me pause. But after thinking about it a bit, I want to note two things about it:

This effect, "the globe disappears when we switch terrain providers", actually happens after the changes in this PR, too. It just might disappear for a briefer period of time. Because even if the terrain provider itself is ready to go, the globe won't appear until the root tiles are downloaded, and the old globe disappears before that happens in either case.
An advanced user can reduce the "no globe" time in the old readyPromise pattern too, making it as short as it is in this PR, if they want to. Something like this (not tested):

const newTerrainProvider = new CesiumTerrainProvider(...);
newTerrainProvider.readyPromise.then(() => {
   viewer.terrainProvider = newTerrainProvider;
});

This comes with all the same dangers as in the forced-async pattern in this PR, of course, but at least in this case it's an advanced user opting in.

We already have a TODO item to fix these kinds of issues in Sandcastle. And as you mentioned, imagery providers will provide a better use-case to discuss the new API with the community.

Ok that's fine but I don't think this is "oh yeah we just need to go back and fix Sandcastle, no big deal." This is exposing a major problem with this pattern, because there is no reasonable solution. Maybe I'm wrong about that, but I think it'd be wise to prove me wrong before spending a lot more time implementing this pattern, because if I'm right we're going to be throwing away a lot of code.

but I think it'd be wise to prove me wrong before spending a lot more time implementing this pattern, because if I'm right we're going to be throwing away a lot of code.

I think it's a fair point to discuss how developers may use the API overall and what additional steps we might consider taking to simplify it, especially at configuration time as @shunter calls out.
Though if we do choose to make adjustments there, I don't think the will mean throwing away the majority of the code in this branch. readyPromise is still an anti-pattern and the team wants to get rid of it throughout the API.

This pattern allows us to more easily update tests such that we can get rid of some asynchronous failures as mentioned in https://github.com/CesiumGS/cesium/issues/10909.
Even assuming we fix all the underlying asynchronous issues in the middle of the stack and the tests, the readyPromise pattern sets a bad precedent as we've seen it become abused in other parts of the code over time.
Even if we do simplify the most common path, why wouldn't we be able to continue to expose this new async API for users who specifically want to opt into a different control flow? There's a use case for this as mentioned in your above comment.

@jjhembd @kring
I was thinking through what we could do to simply terrain (and eventually something similar for imagery) provider creation for users who do not want to dive into the async flow, while still preserving the .fromXYZ factory functions for those users who do. While the approach is different then what @kring was suggesting with the RasterOverlay system, here's the start of what I was thinking. Terrain is a new object that handles the async error handling and exposes an error event and a ready event. One plus here is it's more compatible with event-driven UIs then readyPromise was.
This is then handled by Viewer (and eventually the base layer picker and perhaps CesiumWidget). This also restores the behavior that exists in main such that the existing terrain immediately disappears when setting the new Terrain object. (This does make the "placeholder" terrainProvider more explicit by setting it to undefined, but I think this make it clearer what's happening under the hood. @jjhemb mentioned this was a point of confusion for him regarding EllipsoidTerrainProvider vs undefined values for the terrain provider.) This also handles "competing" values, like when setting a terrain provider and then immediately setting another.
One downside of this approach is that (without TypeScript) it's not possible to confirm that a promise resolves to the expected type during object creation. However, there are many other areas of the code where this is the case, and hopefully won't be a concern if/when we move to TypeScript. I believe this wouldn't be an issue with the RasterOverlay approach, but I think the approach here is a bit less drastic of a change to the API overall, which was the intention.

That's interesting @ggetz, but it's still a big step back from both the current implementation and my proposed factory (raster overlay) implementation. Specifically:

After calling setTerrain, there's no outward appearance on Globe/CesiumWidget/Viewer that anything is happening, and
If a user calls setTerrain, and then sets the terrainProvider explicitly, the latter action will eventually get clobbered by the former one. It's a less likely race condition, but still a fairly annoying one.

Also lots of Cesium apps can and should avoid using Viewer, so it's inappropriate to put this logic there IMO. But that's probably easy to fix.
I really don't think the factory is a big lift, especially when we're already creating factories (Builder). There's no reason to rush to weaker designs just to avoid it. I guess I just need to prototype it to demonstrate that.

After calling setTerrain, there's no outward appearance on Globe/CesiumWidget/Viewer that anything is happening

There should be. That example implements the same "the globe disappears when we switch terrain providers" visual behavior as the existing API. Is there another visual indicator you were looking for that the terrain provider is changing?

If a user calls setTerrain, and then sets the terrainProvider explicitly, the latter action will eventually get clobbered by the former one. It's a less likely race condition, but still a fairly annoying one.

Good point, but this seems like a fairly straightforward fix– Remove the ready event listener if terrainProvider is set directly after setTerrain is called.

Also lots of Cesium apps can and should avoid using Viewer, so it's inappropriate to put this logic there IMO. But that's probably easy to fix.

I agree that most apps should not be using viewer. But as I stated in the comment this does not need to be exclusive to the viewer. We would likely add this functionality to CesiumWidget as well, and in doing so it probably makes sense to move the main logic there. I was hoping this abstraction could live entirely on the Widget layer, but it could be argued this may instead belong in scene. But as you mention, that's an easy fix.
But I don't see why we shouldn't expose the functionality in Viewer at all though though, especially since it already exposes setting the terrainProvider property.


Size of activity bar cannot be smaller than the view column rather than a day minimum

Hi guys, great stuff on this. I'm not proficient on github, so I thought this was the best place for my suggestion.
Just a small thing, I found that when the options match the view type i.e. "Month", "Week", the bar size cannot shrink down to less than that period.
I found that it is down to the single line in update_bar_position() .. where the last if statement has;
If(width && width >= this.gantt.options.column_width) ...
This probably shouldn't concern itself with the column width unless you start dividing it to get the day resolution. But the get_snap.. function run prior does all of this for you, I think its probably just best to leave it as;
If(width && width >= 0)
And let the function prior to its call manage the minimum sizes and snaps.
Cheers,
Pete

Fixed today ;).


Update Composer dependencies (2024-04-10-00-17)

Visual regression test passed!
View the visual regression test report


Too little space around the "Array of text and icon" component

Some places there is not enough space. Like the following screen example:

@satyendra900 The top/bottom space is set to 3XL for the array of text/icon component. Let's try to increase to 4XL.

@BirteThornquist Done! Please check.

@satyendra900 Looking good to me!