Allow

Suggestion

🔍 Search Terms

boolean refinement

Viability Checklist

My suggestion meets these guidelines:

  • This wouldn’t be a breaking change in existing TypeScript/JavaScript code
  • This wouldn’t change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn’t a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript’s Design Goals.

Suggestion

Boolean(x) && x.y should be identical to !!x && x.y as far as the type system is concerned

💻 Use Cases

use case: boolean return value along with style guides that promote Boolean(val) vs !!val

function foo(x: {y: boolean} | null): boolean {
    return !!x && x.y;
}

function foo2(x: {y: boolean} | null): boolean {
    return Boolean(x) && x.y; // error on x.y, x is possibly null
}

The one issue I could theoretically see is someone weirdly overwriting Boolean and introducing side-effects. But talk about edge cases!

I can re-write this as Boolean(x?.y), but that’s not as clear IMO.

1 thought on “Allow

Comments are closed.

Allow

I’ve come across some unexpected behaviour using the vec![] macro. It allows the use of both an elements and repeat syntax like so…

let a = vec![1, 2, 3]; // [1, 2, 3]
let b = vec![1; 3]; // [1, 1, 1]

However, it doesn’t appear to allow a combination of the two like this…

let c = vec![1, 2; 3]; // [1, 2, 2, 2]

…or even this…

let d = vec![1, 2; 3, 4]; // [1, 2, 2, 2, 4]

I’m unsure as to whether this is the intended behaviour of the macro and to whether this is a bug or feature request that I should create an RFC for?

It appears logical and useful to me that the macro should also support the latter cases. Apologies if this has already been brought up, I couldn’t find anything about this.

1 thought on “Allow

  1. let c = vec![1, 2; 3]; // [1, 2, 2, 2]

    It’s not obvious to me that this should result in [1, 2, 2, 2] and not [1, 2, 1, 2, 1, 2]

Comments are closed.

Allow

The following code:

fn main() {
    #[attr] if let Some(_) = Ok(true) {}
}

produces this error:

error: attributes are not yet allowed on `if` expressions
 --> src/main.rs:2:5
  |
2 |     #[attr] if let Some(_) = Ok(true) {}
  |     ^^^^^^^

error: aborting due to previous error

Proc macros like pin-project currently use a trick to work around attributes on expressions being unstable: The entire function is annotated with a ‘wrapper’ attribute, which goes through and manually parses expression attributes. For example:

#[my_attr]
fn foo() {
    #[my_attr] let a = 25;
}

In the example above, the #[my_attr] let a = 25; will be processed by the outer #[my_attr] fn foo() implementation, which will strip away the expression-based #[my_attr] before emitting the final TokenStream.

Unfortunately, the current ‘attributes are not yet allowed on if expressions’ check is done during parsing:

self.error_attr_on_if_expr(&expr);

While the proc-macro itself can parse the attribute on the if let without any errors, compilation will still fail due to the parsing error that was emitted before any proc-macros were run.

It would be extremely useful if this check were to be moved until after parsing, so that proc macros had a chance to remove attributes from if expressions.

1 thought on “Allow

  1. Hmm; seems like the concern was primarily semantic (in terms of macros & lints) as opposed to syntactic?

Comments are closed.

Allow

I’ve been using create-react-app in conjunction with lerna repos and for the most part its been an awesome experience. The one issue I’ve been seeing is when trying to issue lerna run test commands to test all packages that have a “test” script. Since create-react-app defaults to entering watch mode on test, it hangs.

After some digging it looks like I might be able to set a CI environment variable to disable the watch mode, however this does not work well due to the nuances of setting environment variables cross platform.

Something as simple as changing the default from

    "test": "react-scripts test --env=jsdom",

to

    "test": "react-scripts test --env=jsdom --watch",

would allow me to simply turn it off. Conversely adding an optional --no-watch or --single-run flag would solve the problem.

I’m happy to submit a PR, but wanted to get some feedback first on the internal direction regarding whether this has been considered in the past, and the reasoning around the current implementation. I get that its a nice experience for the casual user, but at the cost of CI and incompatibility with lerna there should be some sort of escape hatch (short of eject).

8 thoughts on “Allow

  1. You can set them in cross-platform way:

    npm i --save-dev cross-env
    

    then use

      "test": "cross-env CI=true react-scripts test --env=jsdom"

    Further, npm test is commonly used in CI processes in the context of it being a single run without needing to set an environment variable.

    I think you might have misunderstood. In most CIs you don’t need to set this variable because it is already set by CI environments. Travis, Circle, and probably others already do this.

    I hope this helps!

  2. From https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/scripts/test.js#L29-L32

    // Watch unless on CI or in coverage mode
    if (!process.env.CI && argv.indexOf('--coverage') < 0) {
      argv.push('--watch');
    }

    From this, cross-env is one solution, but running npm test -- --coverage also only runs tests once, and avoids the need for another dependency (unless you already have need for it for other reasons).

    For my projects I’ve created an additional NPM script:

    "test:coverage": "npm test -- --coverage"

    and just include that in other script definitions as needed when I want to run tests only once.

  3. and probably others already do this.

    Jenkins does not set CI env variable.

    Also setting up debug launch configuration in VS Code for tests is extra effort. Actually I am not yet figure it out.

Comments are closed.

1 thought on “Allow

  1. This just seems like a very simple change that makes sense in isolation and will improve our examples / guide etc.

Comments are closed.