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

[demo / bug] inline optimization is broken on subsequent page load #495

Closed
wants to merge 1 commit into from
Closed

[demo / bug] inline optimization is broken on subsequent page load #495

wants to merge 1 commit into from

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Mar 27, 2021 •
edited
Loading

Related Issue

related to #447, this is an example of the inline optimization setting. Click here for the simpler diff. This is a known broken issue since it works in the specs, but not with the Greenwood

Score seems about the same?

Network requests (not entirely everything is inlined, but most)
optimization-inline-network

Lighthouse
optimization-inline-lighthouse

On Navigation 😬
optimization-inline-route-change

Summary of Changes

  1. Sets optimization setting to inline in greenwood.config.js
  2. Set <base href="/"> to set consistent reference point for all JS / CSS file requests

@thescientist13 thescientist13 self-assigned this Mar 28, 2021
@thescientist13 thescientist13 mentioned this pull request Mar 28, 2021
Merged
16 tasks
@thescientist13 thescientist13 added the question Further information is requested label Mar 28, 2021
@thescientist13 thescientist13 added bug Something isn't working CLI todos and removed question Further information is requested labels Apr 17, 2021
@thescientist13 thescientist13 mentioned this pull request Apr 25, 2021
Merged
4 tasks
@thescientist13 thescientist13 changed the base branch from release/0.10.0 to master April 25, 2021 17:16
@thescientist13 thescientist13 changed the base branch from master to release/0.10.0 April 25, 2021 17:16
@thescientist13 thescientist13 force-pushed the enhancement/issue-354-restore-optimization-for-no-bundle-inline branch from 952e614 to d2907bb Compare April 29, 2021 01:04
@thescientist13 thescientist13 changed the base branch from release/0.10.0 to master April 29, 2021 01:04
@thescientist13
Copy link
Member Author

looking at this some more now after syncing with master, getting new errors related to double component definition errors?
Screen Shot 2021-04-28 at 9 04 08 PM

@thescientist13
Copy link
Member Author

thescientist13 commented Jul 2, 2021 •
edited
Loading

Ok, so following the first error at least (unexpected end of input) on /about page for example, it looks like something is happening for pages just using the shelf and attributable to this section of the shelf code

Screen Shot 2021-07-02 at 1 10 36 PM

  expandRoute(path) {
    let routeShelfListIndex = this.shelfList.findIndex(item => {
      let expRoute = new RegExp(`^${path}$`);
      return expRoute.test(item.route);
    });

As can be seen, it looks like a new HTML document is getting loaded right in the middle of that regex in the minified and inlined code, which appears to be another full copy of the page's HTML? 😳

expandRoute(e){let t=this.shelfList.findIndex(t=>new RegExp(`^${e}<!DOCTYPE html><html lang="en" prefix="og:http://ogp.me/ns#">

@thescientist13 thescientist13 changed the title [demo] inline optimization is broken on subsequent page load [demo / bug] inline optimization is broken on subsequent page load Jul 2, 2021
@thescientist13 thescientist13 added the website Tasks related to the projects website / documentation label Jul 2, 2021
@thescientist13
Copy link
Member Author

The plot thickens... checked a couple more things

  • When checking the .greenwood/ directory, the HTML files are fine, so it seems to indicate the issue happens after prerendering (Puppeteer), which really only leaves the bundle step (Rollup)
  • If I check the source of the bundled shelf.js in the public/ directory (as bundled by Rollup) the Regex is fine!

Screen Shot 2021-07-07 at 1 35 43 PM

So, basically it must be isolated to what is happening in our rollup.config.js, but specifically in the inlining logic. Let me setup a test environment know that I've been able to narrow things down.

I probably botched a regex or replace call somewhere...

On an unrelated note, I'm not sure we actually need the ^ and the $ since we're not using multiline, but I'll revisit that a bit later.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#advanced_searching_with_flags

The m flag is used to specify that a multiline input string should be treated as multiple lines. If the m flag is used, ^ and $ match at the start or end of any line within the input string instead of the start or end of the entire string.

@thescientist13
Copy link
Member Author

thescientist13 commented Jul 7, 2021 •
edited
Loading

Dang, so I think this is the issue
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#specifying_a_string_as_a_parameter

$` | Inserts the portion of the string that precedes the matched substring.

Essentially, the Regex inside our source code is getting interpreted by replace as a parameter it should do something with. :/

Looks like there is workaround here
https://stackoverflow.com/a/22612228/417806

So I guess would need to go through and check all usages of replace to make sure we're handling source code appropriately.

@thescientist13 thescientist13 mentioned this pull request Jul 7, 2021
Closed
5 tasks
@thescientist13
Copy link
Member Author

Ok. moved to an issue now that I can reproduce in a spec locally.

@thescientist13 thescientist13 linked an issue Jul 7, 2021 that may be closed by this pull request
inline optimization string replace calls that have a lookahead Regex inside a template literal ($) in the replacement string breaks users pages #656
Closed
5 tasks
@thescientist13 thescientist13 deleted the enhancement/issue-354-restore-optimization-for-no-bundle-inline branch December 18, 2022 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thescientist13 thescientist13

Labels
bug Something isn't working CLI Router website Tasks related to the projects website / documentation
Projects
None yet
Milestone
No milestone
1 participant
@thescientist13