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.

Sign up for GitHub

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

Jump to bottom

update begin adapter to new app-utils format #220

Merged
merged 10 commits into from
Feb 25, 2021
Merged

update begin adapter to new app-utils format #220

merged 10 commits into from
Feb 25, 2021

Conversation

antony
Copy link
Member

@antony antony commented Dec 3, 2020 •
edited
Loading

This is a partial rewrite of the begin adapter which appears to now be fully working with the exception of:

Once static assets are working, this should be good to go.

@antony antony force-pushed the feature/begin-adapter-rewrite branch 2 times, most recently from a4eb8b0 to 8606160 Compare December 9, 2020 19:30
@benmccann
Copy link
Member

@antony just fyi, it looks like this PR will need to be rebased

@Rich-Harris Rich-Harris mentioned this pull request Dec 11, 2020
Merged
@antony antony force-pushed the feature/begin-adapter-rewrite branch from 8606160 to dce3cd3 Compare December 11, 2020 12:53
@benmccann benmccann added the adapters - general Support for functionality general to all adapters label Dec 11, 2020
@antony antony force-pushed the feature/begin-adapter-rewrite branch from dce3cd3 to 3a9a293 Compare January 7, 2021 07:57
@benmccann benmccann linked an issue Jan 7, 2021 that may be closed by this pull request
Begin adapter cleanup and fixes #175
Closed
@benmccann
Copy link
Member

@antony it looks like you need to run prettier to make lint pass

@antony antony force-pushed the feature/begin-adapter-rewrite branch from 3a9a293 to 94e63c5 Compare January 13, 2021 23:39
@antony
Copy link
Member Author

antony commented Jan 22, 2021

So, this PR is now really only blocked by #324 - if anybody can clarify my understanding of that, or if there needs to be a fix, we should be ready to go.

@antony
Copy link
Member Author

antony commented Jan 25, 2021

Just to detail the static assets issue:

When "adaptors" transform the output, they place static assets into one location, lambdas into another location, manifests and shared stuff into a location, etc.

On begin, static assets are only served statically from _static. This isn't where the app looks for these files, so all static assets are missing.

We can use fingerprinting to build a manifest of assets, and then iterate these to serve them up from the lambda, but this is inefficient, and also doesn't work, since the manifest of assets is built at deploy time, this is after the adaptor is run.

it's been a while since I worked on this due to the other blocking issue which is a dependency issue. However, I did spend a long time trying to get to the end goal which was static asset (js, the public dir etc) being served from / if they existed, and otherwise serving up the lambda for requests, and I couldn't do it.

benmccann
benmccann reviewed Feb 10, 2021
View reviewed changes
packages/adapter-begin/index.js
const parse = require('@architect/parser');
const child_process = require('child_process');
const { prerender, generate_manifest_module } = require('@sveltejs/app-utils/renderer');
const { readFileSync, existsSync } = require('fs');
Copy link
Member

Choose a reason for hiding this comment

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

Assuming #379 is merged, you'll want to convert this to ES6. I didn't touch the Begin adapter to avoid a merge conflict with this PR

@antony antony force-pushed the feature/begin-adapter-rewrite branch from 4891291 to b9b0d50 Compare February 25, 2021 07:40
@antony
Copy link
Member Author

antony commented Feb 25, 2021

Merging so that this can be kept up to date more easily.

@antony antony merged commit f3e7665 into master Feb 25, 2021
@antony antony deleted the feature/begin-adapter-rewrite branch February 25, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmccann benmccann benmccann left review comments

@Rich-Harris Rich-Harris Rich-Harris left review comments

Assignees
No one assigned
Labels
adapters - general Support for functionality general to all adapters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Begin adapter cleanup and fixes
3 participants
@antony @benmccann @Rich-Harris