Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[legacy-framework] Add support for blitz.config.ts #1325

Closed
wants to merge 27 commits into from

Conversation

jamiedavenport
Copy link

@jamiedavenport jamiedavenport commented Oct 14, 2020

Closes: blitz-js/legacy-framework#632

What are the changes and their implications?

Checklist

  • Tests added for changes
  • PR submitted to blitzjs.com for any user facing changes

@jamiedavenport
Copy link
Author

A couple of things to note:

@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2020

Size Change: +1.35 kB (0%)

Total Size: 193 kB

Filename Size Change
packages/config/dist/config.cjs.development.js 1.4 kB +48 B (3%)
packages/config/dist/config.cjs.production.min.js 943 B +26 B (2%)
packages/config/dist/config.esm.js 1.33 kB +49 B (3%)
packages/file-pipeline/dist/file-pipeline.cjs.development.js 5.96 kB +74 B (1%)
packages/file-pipeline/dist/file-pipeline.cjs.production.min.js 3.06 kB +49 B (1%)
packages/file-pipeline/dist/file-pipeline.esm.js 5.81 kB +58 B (0%)
packages/server/dist/server.cjs.development.js 21.3 kB +415 B (1%)
packages/server/dist/server.cjs.production.min.js 12.3 kB +210 B (1%)
packages/server/dist/server.esm.js 21.4 kB +424 B (1%)
ℹ️ View Unchanged
Filename Size Change
packages/blitz/dist/cli.js 15.7 kB 0 B
packages/blitz/dist/index.js 245 B 0 B
packages/config/dist/index.js 143 B 0 B
packages/core/dist/core.cjs.development.js 12.9 kB 0 B
packages/core/dist/core.cjs.production.min.js 6.98 kB 0 B
packages/core/dist/core.esm.js 12.5 kB 0 B
packages/core/dist/index.js 141 B 0 B
packages/display/dist/display.cjs.development.js 1.43 kB 0 B
packages/display/dist/display.cjs.production.min.js 819 B 0 B
packages/display/dist/display.esm.js 1.33 kB 0 B
packages/display/dist/index.js 144 B 0 B
packages/file-pipeline/dist/index.js 147 B 0 B
packages/generator/dist/generator.cjs.development.js 15.7 kB 0 B
packages/generator/dist/generator.cjs.production.min.js 9.51 kB 0 B
packages/generator/dist/generator.esm.js 15.4 kB 0 B
packages/generator/dist/index.js 145 B 0 B
packages/generator/dist/templates/app/babel.config.js 77 B 0 B
packages/generator/dist/templates/app/blitz.config.js 324 B 0 B
packages/generator/dist/templates/app/jest.config.js 682 B 0 B
packages/generator/dist/templates/app/test/__mocks__/fileMock.js 54 B 0 B
packages/installer/dist/index.js 145 B 0 B
packages/installer/dist/installer.cjs.development.js 7.69 kB 0 B
packages/installer/dist/installer.cjs.production.min.js 5.14 kB 0 B
packages/installer/dist/installer.esm.js 7.54 kB 0 B
packages/repl/dist/index.js 141 B 0 B
packages/repl/dist/repl.cjs.development.js 1.81 kB 0 B
packages/repl/dist/repl.cjs.production.min.js 1.08 kB 0 B
packages/repl/dist/repl.esm.js 1.69 kB 0 B
packages/server/dist/index.js 142 B 0 B

compressed-size-action

@ryardley
Copy link
Collaborator

ryardley commented Oct 14, 2020

Sweet thanks for looking at this. I would recommend having a look at the workoptimizer test https://github.com/blitz-js/blitz/blob/5572c9c3239b7beb001cec1914e4b59fe5ee10c2/packages/file-pipeline/src/helpers/work-optimizer/work-optimizer.test.ts and writing one for the config stage.

The other stages need to be done too but that is out of the scope of this PR.

@ryardley
Copy link
Collaborator

This solution copies the config over into the root as _blitz.config.js because I was experiencing this problem (#726) but I'm sure there must be a more elegant way.

Not sure I understand what is going on here? Sounds to me like we probably shouldn't have to do this.

Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet!!

First thing: we must also be able to import other .ts files into blitz.config.ts and have it work correctly. I don't think this will work right now with this pr, right? Because for this to work, we basically need to run webpack on the config file to bundle it, right? I should have thought about and mentioned this in slack when we were talking about just using babel.

As for where to put the built file... My first thought is to put it at .next/__blitz.config.js because that's where we are bundling the db/index.ts file to as .next/__db.js. We can ensure that the .js blitz config always lives there, and then we can change @blitzjs/config to read from there. And then also the config stage would need changed to read from there too.

@@ -29,6 +29,7 @@
"module": "dist/server.esm.js",
"types": "dist/packages/server/src/index.d.ts",
"dependencies": {
"@babel/core": "7.11.6",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a ^ range here so that we use the same version Next.js. Or, even we could maybe remove it from here since next includes it.

@flybayer
Copy link
Member

ncc would be the easy way to bundle blitz.config.ts

if (isBlitzTsConfigPath(file.path)) {
const res = await bundle(file.path, {
quiet: true,
externals: ["@blitzjs/server", "@next/bundle-analyzer"], // FIXME: Figure out why this wouldn't work? Do we need to bundle everything?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to do here. ncc failed to bundle these 3rd party modules, although it shouldn't have to because they're already built. ncc doesn't seem to provide a nice way to do this (vercel/ncc#409) so I'm considering:

  • Maintain this list manually, thinking that there shouldn't be a lot of 3rd party modules that the config will be expected to import (mostly just auth stuff?)
  • Programmatically get all installed modules with yarn list --json or similar

What are your thoughts? @flybayer

@flybayer
Copy link
Member

@jamiedavenport sorry I haven't had time to dig into this yet. Had some other life stuff interfere this week and some high priority bugs I'm working through. I'll get back to this as soon as I can.

@jamiedavenport
Copy link
Author

@ryardley @flybayer would either of you guys be able to review this soon? 😃

@flybayer
Copy link
Member

@jamiedavenport yeah, so sorry. I've been focused 100% on fixing a bunch of bugs for official beta status, and new features are a bit lower priority than that.

This week I'm very busy and have vacation, so hopefully I can give this full attention in first part of December.

@flybayer
Copy link
Member

I need to really think through all potential edge cases and also test in multiple production deployments (Vercel and Render).

@flybayer
Copy link
Member

NOTE: also need to update recipes to work with blitz.config.ts

@flybayer
Copy link
Member

@jamiedavenport I'm really sorry I let this drop. I think subconsciously I knew ncc wasn't the right approach here (but at the time I didn't understand some of these tools well enough to know what else to do).

I've now implemented this using esbuild in a much cleaner way: #2283

I really, really appreciate the effort you put into this. I know it's a bummer to put a lot of effort into something that doesn't even get used. So for that I'm sorry 🙏

@flybayer flybayer closed this Apr 29, 2021
@dillondotzip dillondotzip changed the title Add support for blitz.config.ts [legacy-framework] Add support for blitz.config.ts Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for blitz.config.ts
4 participants