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

Allow pre-parsing of ESI document, add example of fetching per-user fragment variants #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kailan
Copy link
Member

@kailan kailan commented Oct 11, 2024

Resolves #3

@kailan kailan marked this pull request as ready for review October 23, 2024 13:42
@kailan kailan added the enhancement New feature or request label Oct 23, 2024
@kailan kailan requested a review from vagetman October 23, 2024 15:35
Copy link
Collaborator

@vagetman vagetman left a comment

Choose a reason for hiding this comment

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

Some minor details

@@ -140,9 +169,8 @@ impl Processor {
);

// `root_task` is the root task that will be used to fetch tags in recursive manner
let root_task = &mut Task::new();
let root_task: &mut Task = &mut Task::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is pretty obvious and no more explicit notation required.

# This file describes a Fastly Compute package. To learn more visit:
# https://developer.fastly.com/reference/fastly-toml/

authors = ["[email protected]"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
authors = ["kailan@enviark.com"]
authors = ["kblanks@fastly.com"]

I think we usually leaving those empty or with fastly emails, no?

Comment on lines +1 to +4
use std::{
collections::{HashMap, VecDeque},
io::Write,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use VecDeque

Suggested change
use std::{
collections::{HashMap, VecDeque},
io::Write,
};
use std::{collections::HashMap, io::Write};
};

let mut xml_writer = Writer::new(output_writer);

// Parse the ESI document and store it in memory
let mut events = VecDeque::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use VecDeque

Suggested change
let mut events = VecDeque::new();
let mut events = Vec::new();

let mut events = VecDeque::new();
let mut beresp_reader = Reader::from_reader(beresp.take_body());
parse_tags("esi", &mut beresp_reader, &mut |event| {
events.push_back(event);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use VecDeque

Suggested change
events.push_back(event);
events.push(event);


// Process the already-parsed ESI document, replacing the request URLs with the variant URLs
let result = processor.process_parsed_document(
events,
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use VecDeque

Suggested change
events,
events.into(),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to pre-parse document for fragment URLs
2 participants