sanctuary-js / sanctuary-site

Sanctuary's website
https://sanctuary.js.org
7 stars 4 forks source link

Note about vite and checkTypes #102

Closed dotnetCarpenter closed 2 years ago

dotnetCarpenter commented 2 years ago

I found a gotcha when using sanctuary and vite, that makes sanctuary about a hundred times slower ;)

According to vite - Env Variables and sanctuary - Type checking, it looks like you automatically disable type checking when you create a vite build using npm run build. But due to the way that vite substitute process.env.NODE_ENV with either development or production and how sanctuary is detecting process.env.NODE_ENV, you will not disable type checking!

The work-around is:

import sanctuary from 'sanctuary';
const S = sanctuary.create ({ checkTypes: import.meta.env.DEV, env: sanctuary.env });

_You can not currently set process.env.NODE_ENV manually in vite. As of writing, vite is version 2.9.6._

I wanted to create a PR for this to update the type-checking or installation section but checking that it looks good is a little more involved than I have time for currently. So this is serve as a reminder :)

dotnetCarpenter commented 2 years ago

BTW. I'm using latest versions of the sanctuary projects, as listed below:

  "dependencies": {
    "fluture": "fluture-js/Fluture#25cbb445250039ac2e1add982264670986f9df45",
    "fluture-sanctuary-types": "^7.1.0",
    "sanctuary": "sanctuary-js/sanctuary#72bc20ad02c5944b661efdcdd14ab60ceb92c7b3",
    "sanctuary-def": "sanctuary-js/sanctuary-def#deadb523c488e8e7bbc988a662c62435be38a380",
    "sanctuary-either": "sanctuary-js/sanctuary-either#18f6238f1d1e8e36d3dcdb44732255b3349eef46",
    "sanctuary-identity": "sanctuary-js/sanctuary-identity#b0df277c80e562616ccd526b56a3d9af69ace56f",
    "sanctuary-maybe": "sanctuary-js/sanctuary-maybe#2d14d53f939c0a81e081d93dc4bda5622ca517a1",
    "sanctuary-pair": "sanctuary-js/sanctuary-pair#829187679afceec52e889f884bdc6e9b0c0d0a18",
    "sanctuary-show": "^3.0.0"
  },
  "overrides": {
    "sanctuary-type-classes": "^13.0.0",
    "sanctuary-show": "^3.0.0",
  },
Avaq commented 2 years ago

Does the documentation need to be updated? This seems to be in line with what's documented:

There is a performance cost to run-time type checking. Type checking is disabled by default if process.env.NODE_ENV is 'production'. If this rule is unsuitable for a given program, one may use create to create a Sanctuary module based on a different rule.

dotnetCarpenter commented 2 years ago

Two things that are not covered is that setting process.env.NODE_ENV in an esm file will not carry over to sanctuary. But will remain undefined. When using esm, environment variables really has to be an environment variable.

Second, in conjunction with vite process.env.NODE_ENV is replaced with the string "production" or "development". That should seemingly work with how sanctuary detect process.env.NODE_ENV:

  return create ({
    checkTypes: typeof process === 'undefined'
                || process == null
                || process.env == null
                || process.env.NODE_ENV !== 'production',
    env: $.env
  });

It gets transpiled into:

pt({
    checkTypes: typeof process == "undefined" || process == null || process.env == null || !1,
    env: t.env
})

🤔 seems legit. I was probably bitten by using process.env.NODE_ENV = 'production' in esm. But that would still set checkTypes to false. I'm honestly not sure what happen when I filed this bug report... 🤷

PS. typeof process === 'undefined' and process == null looks like a duplication of logic...

davidchambers commented 1 year ago

PS. typeof process === 'undefined' and process == null looks like a duplication of logic...

I agree that it looks duplicative, but we cannot just use process == null because it would fail if process is not defined. We could make the second clause process === null for clarity, but that is not consistent with our ESLint configuration.