dart_frog
dart_frog copied to clipboard
feat: Add port number feature to build command
Status
READY
Description
Added the port number to the build option for #1327
Type of Change
- [x] β¨ New feature (non-breaking change which adds functionality)
- [ ] π οΈ Bug fix (non-breaking change which fixes an issue)
- [ ] β Breaking change (fix or feature that would cause existing functionality to change)
- [ ] π§Ή Code refactor
- [ ] β Build configuration change
- [ ] π Documentation
- [ ] ποΈ Chore
Hi @plato79 πthanks for taking on the issue and opening a PR! It looks like there are some failures in CI so please take a look at the output and get everything to green before we start the review process. Thanks!
The only "problem" was the initialization of "vars" variable which I didn't change. I fixed it as final. It should now pass ci analyze step. Though some of the tests fails and I don't think it's because of my additions.
Now, it's past anlalyze.. Though there is an error in prod_server_builder test. This one is a bit complex though, while the fix is very easy.
The code below is the reason for the error.
prod_server_builder.dart:
final vars = <String, dynamic>{
'dartVersion': dartVersion,
'port': port,
};
logger.detail('[codegen] running pre-gen...');
await prodServerBundleGenerator.hooks.preGen(
vars: vars,
workingDirectory: workingDirectory.path,
onVarsChanged: (v) => vars..addAll(v ?? {}),
);
There is a vars variable which has default dartVersion
which is "stable"
and default port
which is "8080"
. There is possibility additional variables are provided through mason. We add those to the current variables defined in vars
we have here. It modifies the value if the same key is used so everything is alright.
...until no variables are sent to here. Which makes v
null
(it says it shouldn't happen but it happens). Why v
has null
value I am not sure. I thought that code would execute only when there is a value in v
but this is not the situation.
Now the fix is easy as you can see. Just adding a null check fixes. But this time flutter says I shouldn't add a null check to a variable which shouldn't be null at all. While the code above fixes the problem and causes no problem, it probably will pop up in the ci analyze section as error just like the previous `"vars" should be defined as final" warning appears as error in analyze.
Do you have any suggestion about this?
Now, it's past anlalyze.. Though there is an error in prod_server_builder test. This one is a bit complex though, while the fix is very easy.
~~The code below is the reason for the error.~~
prod_server_builder.dart:
final vars = <String, dynamic>{ 'dartVersion': dartVersion, 'port': port, }; logger.detail('[codegen] running pre-gen...'); await prodServerBundleGenerator.hooks.preGen( vars: vars, workingDirectory: workingDirectory.path, onVarsChanged: (v) => vars..addAll(v ?? {}), );
~~There is a vars variable which has default
dartVersion
which is"stable"
and defaultport
which is"8080"
. There is possibility additional variables are provided through mason. We add those to the current variables defined invars
we have here. It modifies the value if the same key is used so everything is alright.~~~~...until no variables are sent to here. Which makes
v
null
(it says it shouldn't happen but it happens). Whyv
hasnull
value I am not sure. I thought that code would execute only when there is a value inv
but this is not the situation.~~~~Now the fix is easy as you can see. Just adding a null check fixes. But this time flutter says I shouldn't add a null check to a variable which shouldn't be null at all. While the code above fixes the problem and causes no problem, it probably will pop up in the ci analyze section as error just like the previous `"vars" should be defined as final" warning appears as error in analyze.~~
Do you have any suggestion about this?
Ok, that was not the cause. Still checking to see what the real problem here is.
This last commit should fix the error in the test.
The failing tests are actually runs successfully if you execute the commands outside of the test. For example build_test fails, but if you run dart run build/bin/server.dart
in the temporary directory it created and call http://localhost:8080
it works. But the test itself cannot get a response from that page.
@tomarra Are you sure that these tests worked without problem before I made the change?
I think this is ready for merge. What do you think?
Hi @plato79! I've added this to the review column for the next iteration (May 18 to May 31). Hopefully we get to review it by then π