[legacy-framework] refactor(server): Added isTypescript explicit flag, config refactored #1014

svobik7 · 2020-09-06T11:20:27Z

This PR exists just because of initial idea (#941) of refactoring server config and try to make them cacheable and in the same time drop the need of prop drilling. As initial idea of caching leads to no benefits this PR just brings some config refactors, code cleanup and small perf optimization flag in server package.

What are the changes and their implications?

keeps isTypescript prop just in server config and server package and removes it from file pipelines package
removes waiting for both next bin and patched bin as we do not switch between dev/prod in runtime
removes some unnecessary lines and clean up the config code

…ry config lines

ryardley · 2020-09-06T13:44:34Z

Hey @svobik7 Thanks for this! I will need a couple of days to look at this properly. Looks good so far.

ryardley

I kind of question why we are adding a flag here and if that is really worth it from a devx perspective? Also it is kind of hard to explain what it is doing and I am not really convinced we need to do it.

ryardley · 2020-09-08T13:09:52Z

packages/server/src/config.ts

-  const git = parseChokidarRulesFromGitignore(resolve(process.cwd(), config.rootFolder))
+export async function normalize(
+  config: ServerConfig,
+  dev: boolean = false,


Any reason why not just add dev to ServerConfig?

There is no special reason for this. It could definitely be inside ServerConfig. In that case I suggest to use it like following:

export type ServerConfig = { env: "dev" | "prod" ... }

While 'prod' is default. What do you think?

Works for me

Done in current commit

ryardley · 2020-09-08T13:11:15Z

packages/server/src/config.ts

  }
 }
+
+async function getNextBin(rootFolder: string, dev: boolean = false) {


Good work putting these together

Thank you 👍

ryardley · 2020-09-08T13:21:22Z

packages/cli/src/commands/start.ts

@@ -24,17 +18,19 @@ export class Start extends Command {
      char: "H",
      description: "Set server hostname",
    }),
+    ts: flags.boolean({
+      description: "Use typescript explicitly (just for performance optimization)",
+    }),


Is this really actually necessary? Seems like a slightly redundant feature when we are only avoiding a fs read at the start of a cli process

The main reason was to find a way to avoid fs.existsSync called in config.ts which just checks if tsconfig.json file exists. I was thinking like following:

As a newcomer to blitz I want to create and start my application with almost zero-config so I run blitz start and everything works as expected.

Once I settled my initial code base (js, ts) I want to avoid any unnecessary "delays" in dev process. So I have got an option to tell blitz "hey I am using TS, do not check it on your own" by running blitz start --ts.

Also this flag can be used in different commands (generate, console) to avoid checking ts implicitly. But honestly I did not make any perf tests and cannot say now if it is worth. So I do not have any compelling reason to keep this flag right now.

I think there are other way more expensive things we are doing as the command starts up that are going to be slower than one file stats read especially if we do any file pipeline work.

Removed in current commit

ryardley · 2020-09-08T13:28:13Z

packages/server/src/config.ts

+
+async function getIsTypescript(rootFolder: string) {
+  const fs = await import("fs")
+  return fs.existsSync(join(rootFolder, "tsconfig.json"))


This doesn't really matter as it is not in stream but why not use a promisified exists function like you might from using promisify with fs.exists. If we had to run concurrent processes later that needed this function it wouldn't block the thread that way also why not just import fs up top?

Importing fs on top: this is related to --ts flag. Once I was thinking about avoiding fs.existsSync I was already thinking about do not import anything unnecessary if you know typescript is set explicitly.

existsSync: it could be definitely done with promisified exists

I see now not importing fs approach is not very benefical hera as fs imported already for other reasons. So I agree to use promisified check and import from fs directly would be better.

I am gonna update this, ok?

Done in current commit

…r env config

ryardley

Looks good to me. I removed the reference to --ts config option in the PR description.

ryardley · 2020-09-11T01:55:38Z

packages/server/src/config.ts

+
+async function getIsTypescript(rootFolder: string): Promise<boolean> {
+  try {
+    await promises.access(join(rootFolder, "tsconfig.json"))


refactor(server): Added isTypescript explicit flag, removed unnecessa…

fc858b5

…ry config lines

svobik7 requested review from aem and ryardley as code owners September 6, 2020 11:20

blitzjs-bot bot added the status/in-review label Sep 6, 2020

blitzjs-bot bot added this to In Review in Dashboard Sep 6, 2020

ryardley reviewed Sep 8, 2020

View reviewed changes

removed non-benefical --ts flag, removed dynamic imports, added serve…

59bb80e

…r env config

ryardley approved these changes Sep 11, 2020

View reviewed changes

Merge branch 'canary' into fix/server-config-refactor

4025411

flybayer merged commit 280a2b5 into blitz-js:canary Sep 11, 2020

Dashboard automation moved this from In Review to Done Sep 11, 2020

blitzjs-bot bot added status/done and removed status/in-review labels Sep 11, 2020

dillondotzip changed the title ~~refactor(server): Added isTypescript explicit flag, config refactored~~ [legacy-framework] refactor(server): Added isTypescript explicit flag, config refactored Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[legacy-framework] refactor(server): Added isTypescript explicit flag, config refactored #1014

[legacy-framework] refactor(server): Added isTypescript explicit flag, config refactored #1014

svobik7 commented Sep 6, 2020 •

edited by ryardley

ryardley commented Sep 6, 2020

ryardley left a comment

ryardley Sep 8, 2020

svobik7 Sep 9, 2020

ryardley Sep 9, 2020

svobik7 Sep 9, 2020

ryardley Sep 8, 2020

svobik7 Sep 9, 2020

ryardley Sep 8, 2020

svobik7 Sep 9, 2020

ryardley Sep 9, 2020

svobik7 Sep 9, 2020

ryardley Sep 8, 2020

svobik7 Sep 9, 2020

svobik7 Sep 9, 2020

ryardley left a comment

ryardley Sep 11, 2020

[legacy-framework] refactor(server): Added isTypescript explicit flag, config refactored #1014

[legacy-framework] refactor(server): Added isTypescript explicit flag, config refactored #1014

Conversation

svobik7 commented Sep 6, 2020 • edited by ryardley

What are the changes and their implications?

ryardley commented Sep 6, 2020

ryardley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryardley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svobik7 commented Sep 6, 2020 •

edited by ryardley