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

feat(gatsby-script): Script component #35403

Merged
merged 64 commits into from
May 19, 2022
Merged

feat(gatsby-script): Script component #35403

merged 64 commits into from
May 19, 2022

Conversation

@tyhopp tyhopp added topic: performance Related to runtime & build performance type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement. topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation) labels Apr 12, 2022
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Apr 12, 2022
@tyhopp tyhopp marked this pull request as draft April 12, 2022 02:15
@tyhopp tyhopp removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Apr 12, 2022
@LekoArts LekoArts added this to the Script Component milestone Apr 12, 2022
@wardpeet
Copy link
Contributor

Quick question, why a different e2e test and not using production-runtime for this?

@tyhopp tyhopp changed the title feat(gatsby-script): Script component implementation feat(gatsby-script): Script component Apr 19, 2022
@tyhopp
Copy link
Contributor Author

tyhopp commented Apr 19, 2022

Quick question, why a different e2e test and not using production-runtime for this?

Separation of concerns, @pieh mentioned this as well and we thought if the extra setup cost of a separate suite becomes a problem we can move these into production runtime

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Added some remarks but overall looks good

packages/gatsby-script/src/gatsby-script.tsx Outdated Show resolved Hide resolved
packages/gatsby-script/src/gatsby-script.tsx Outdated Show resolved Hide resolved
packages/gatsby-script/src/gatsby-script.tsx Show resolved Hide resolved
packages/gatsby-script/src/gatsby-script.tsx Show resolved Hide resolved
tyhopp and others added 16 commits May 9, 2022 10:47
* test(gatsby-script): e2e test scaffolding

* test(gatsby-script): Test pre-hydrate strategy

* test(gatsby-script): Refactor resource record command

* test(gatsby-script): Test post-hydrate strategy

* test(gatsby-script): Test idle strategy

* test(gatsby-script): Update readme, npm scripts

* test(gatsby-script): Remove unnecessary dev Cypress config

* test(gatsby-script): DRY middleware intercepts

* chore(gatsby-script): e2e circle ci config

* chore(gatsby-script): Add e2e to circle ci jobs

* chore(gatsby-script): Use published package so e2e tests can resolve it

* test(gatsby-script): Add missing network waits to e2e tests
* feat(gatsby-script): Implement inline scripts

* test(gatsby-script): Inline scripts unit tests

* test(gatsby-script): Set up for e2e tests

* test(gatsby-script): Display scripts with sources strategy

* test(gatsby-script): dangerouslySetInnerHTML e2e tests

* test(gatsby-script): Template literals e2e tests

* chore(gatsby-script): Update package readme

* chore(gatsby-script): Install latest canary to run in CI

* chore(gatsby-script): Try waitForRouteChange to fix inconsistent Cypress network waits

* chore(gatsby-script): Try delaying responses so Cypress has time to set up network waits

* chore(gatsby-script): Try increasing network response delay even further

* chore(gatsby-script): Remove unreliable Cypress network waits entirely
* feat(gatsby-script): Normal attribute implementation

* chore(gatsby-script): Bump default command timeout
* feat(gatsy-script): Initial on load callback implementation

* test(gatsby-script): Refactor e2e test structure

* refactor(gatsby-script): Fix eslint, ts errors

* feat(gatsby-script): Handle edge cases from Gatsby link navigation

* test(gatsby-script): Adjust e2e tests so they run successfully again

* feat(gatsby-script): On load callback e2e tests

* test(gatsby-script): Scripts with sources navigation e2e tests

* test(gatsby-script): Inline scripts navigation e2e tests

* test(gatsby-script): Adjust e2e tests, add comment on edge case

* test(gatsby-script): Fix unit tests
* feat(gatsby-script): On error callback

* test(gatsby-script): Update on error callback e2e test logic

* chore(gatsby-script): Skip inline scripts gatsby link nav test
tyhopp and others added 14 commits May 16, 2022 09:43
* feat(gatsby): initial support for off-main-thread in develop

Co-authored-by: tyhopp <[email protected]>

* feat(gatsby): Refactor partytown local proxy logic

* Use origin instead of host in serve/develop proxy

Co-authored-by: Michal Piechowiak <[email protected]>
…me suites (#35663)

* test(gatsby-script): Production runtime

* Develop runtime
* test(gatsby-script): SSR and browser APIs

Co-authored-by: Jude Agboola <[email protected]>

* Development runtime

Co-authored-by: Jude Agboola <[email protected]>
* test(gatsby-script): e2e off-main-thread develop runtime

* Test in CI

* Production runtime

* Update load plugins snapshots again

* Post install Chromium for Playwright

* Fix tests
* add shim and test

* add develop and prod run time e2e tests

* review updates

* Fix imports

* Fix prod runtime imports

* Namespace tests

Co-authored-by: tyhopp <[email protected]>
@tyhopp tyhopp marked this pull request as ready for review May 18, 2022 03:23
@tyhopp tyhopp requested a review from pieh May 18, 2022 04:01
…cripts (#35692)

* feat(gatsby-script): Apply crossorigin anonymous on off-main-thread scripts

* Fix offline test flake
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Let's ship it.

There is follow up to do (handling onLoad and onError callbacks on duplicate scripts as well as some additional warnings for DX - this will be handled in #35610 that aim to be merged before next stable release)

import proxy from "express-http-proxy"
import type { RequestHandler } from "express"

export const partytownProxyPath = `/__partytown-proxy`
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call it something in this vein

Suggested change
export const partytownProxyPath = `/__partytown-proxy`
export const partytownProxyPath = `/__thirdparty-proxy`

}
}
} else {
console.log(`unexpected shape of forward`, newScript)
Copy link
Contributor

Choose a reason for hiding this comment

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

Only log in develop

Suggested change
console.log(`unexpected shape of forward`, newScript)
if (process.env.NODE_ENV === 'development') {
console.log(`unexpected shape of forward`, newScript)
}

Comment on lines +82 to +84
cy.get(`table[id=script-mark-records] tbody`)
.children()
.should(`have.length`, 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

This counts the rows of the table, and this is how you know how many scripts got loaded?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is part assertion and I think part flake potential removal as this will (potentially?) wait for expected amount of scripts to report their status before doing assertion on those individual ones

@@ -0,0 +1,3 @@
{
"presets": [["babel-preset-gatsby-package", { "browser": true }], "@babel/preset-typescript"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need esm to be true too, to make sure it's going to output smaller bundle size

Suggested change
"presets": [["babel-preset-gatsby-package", { "browser": true }], "@babel/preset-typescript"]
"presets": [["babel-preset-gatsby-package", { "browser": true, "esm": true }], "@babel/preset-typescript"]

})
break
case ScriptStrategy.offMainThread:
if (typeof window !== `undefined` && collectScript) {
Copy link
Contributor

Choose a reason for hiding this comment

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

useEffects only run on the client so window is always defined.

Suggested change
if (typeof window !== `undefined` && collectScript) {
if (collectScript) {

return (
<script
type="text/partytown"
async
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async

return (
<script
type="text/partytown"
async
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async

@pieh
Copy link
Contributor

pieh commented May 19, 2022

We will address Ward's comments in follow up as they are not blocking. Merging

@pieh pieh merged commit a88703f into master May 19, 2022
@pieh pieh deleted the feat-script-component branch May 19, 2022 13:01
pieh added a commit that referenced this pull request May 20, 2022
refactor(gatsby,gatsby-script): Misc comments from Ward in #35403

Co-authored-by: Michal Piechowiak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation) topic: performance Related to runtime & build performance type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants