Merge branch 'main' into pr/563

* main: Allow frames to scroll smoothly into view (hotwired#607) Export Type declarations for `turbo:` events (hotwired#452) Add .php as a valid isHTML extension (hotwired#629) Add original click event to 'turbo:click' details (hotwired#611) Drive Browser tests with `playwright` (hotwired#609) Allow Turbo Streams w/ GET via `data-turbo-stream` (hotwired#612) Only update history when Turbo visit is renderable (hotwired#601) Support development ChromeDriver version overrides (hotwired#606) Turbo stream source (hotwired#415) Expose Frame load state via `[complete]` attribute (hotwired#487) fix(ie/edge): form.method='delete', raises Invalid argument. (hotwired#586) Do not declare global types/constants (hotwired#524) Defensively create custom turbo elements (hotwired#483) Use `replaceChildren` in StreamActions.update (hotwired#534)
feliperaul · Jul 16, 2022 · ec38cee · ec38cee
2 parents 6ded8c5 + 39fe418
commit ec38cee
Show file tree

Hide file tree

Showing 62 changed files with 4,439 additions and 3,951 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -7,6 +7,7 @@ jobs:
 
     runs-on: ubuntu-latest
 
+
     steps:
     - uses: actions/checkout@v3
     - uses: actions/setup-node@v3
@@ -18,22 +19,33 @@ jobs:
         key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
 
     - run: yarn install
+    - run: yarn run playwright install --with-deps
     - run: yarn build
 
     - name: Set Chrome Version
       run: |
         CHROMEVER="$(chromedriver --version | cut -d' ' -f2)"
         echo "Actions ChromeDriver is $CHROMEVER"
-        CONTENTS="$(jq '.tunnelOptions.drivers[0].name = "chrome"' < intern.json)"
-        CONTENTS="$(echo ${CONTENTS} | jq --arg chromever "$CHROMEVER" '.tunnelOptions.drivers[0].version = $chromever')"
-        echo "${CONTENTS}" > intern.json
-        cat intern.json
+        echo "CHROMEVER=${CHROMEVER}" >> $GITHUB_ENV
 
     - name: Lint
       run: yarn lint
 
-    - name: Test
-      run: yarn test
+    - name: Unit Test
+      run: yarn test:unit
+
+    - name: Chrome Test
+      run: yarn test:browser --project=chrome
+
+    - name: Firefox Test
+      run: yarn test:browser --project=firefox
+
+    - name: Upload test results
+      if: always()
+      uses: actions/upload-artifact@v2
+      with:
+        name: playwright-report
+        path: playwright-report
 
     - name: Publish dev build
       run: .github/scripts/publish-dev-build '${{ secrets.DEV_BUILD_GITHUB_TOKEN }}'

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -34,18 +34,46 @@ Once you are done developing the feature or bug fix you have 2 options:
 2. Run a local webserver and checkout your changes manually
 
 ### Testing
-The library is tested by running the test suite (found in: `src/tests/*`) against headless browsers. The browsers are setup in `intern.json` check it out to see the used browser environments.
+The library is tested by running the test suite (found in: `src/tests/*`) against headless browsers. The browsers are setup in [intern.json](./intern.json) and [playwright.config.ts](./playwright.config.ts). Check them out to see the used browser environments.
+
+To override the ChromeDriver version, declare the `CHROMEVER` environment
+variable.
+
+First, install the drivers to test the suite in browsers:
+
+``bash
+yarn playwright install  --with-deps
+```
 
 The tests are using the compiled version of the library and they are themselves also compiled. To compile the tests and library and watch for changes:
 
 ```bash
 yarn watch
 ```
 
-To run the tests:
+To run the unit tests:
+
+```bash
+yarn test:unit
+```
+
+To run the browser tests:
+
+```bash
+yarn test:browser
+```
+
+To run the browser suite against a particular browser (one of
+`chrome|firefox`), pass the value as the `--project=$BROWSER` flag:
+
+```bash
+yarn test:browser --project=chrome
+```
+
+To run the browser tests in a "headed" browser, pass the `--headed` flag:
 
 ```bash
-yarn test
+yarn test:browser --project=chrome --headed
 ```
 
 ### Test files
@@ -55,14 +83,23 @@ The html files needed for the tests are stored in: `src/tests/fixtures/`
 
 ### Run single test
 
-To focus on single test grep for it:
-```javascript
-yarn test --grep TEST_CASE_NAME
+To focus on single test, pass its file path:
+
+```bas
+yarn test:browser TEST_FILE
 ```
 
-Where the `TEST_CASE_NAME` is the name of test you want to run. For example:
-```javascript
-yarn test --grep 'triggers before-render and render events'
+Where the `TEST_FILE` is the name of test you want to run. For example:
+
+```base
+yarn test:browser src/tests/functional/drive_tests.ts
+```
+
+To execute a particular test, append `:LINE` where `LINE` is the line number of
+the call to `test("...")`:
+
+```bash
+yarn test:browser src/tests/functional/drive_tests.ts:11
 ```
 
 ### Local webserver

diff --git a/intern.json b/intern.json
@@ -1,6 +1,5 @@
 {
   "suites": "dist/tests/unit.js",
-  "functionalSuites": "dist/tests/functional.js",
   "environments": [
     {
       "browserName": "chrome",

diff --git a/package.json b/package.json
@@ -35,12 +35,14 @@
     "access": "public"
   },
   "devDependencies": {
+    "@playwright/test": "^1.22.2",
     "@rollup/plugin-node-resolve": "13.1.3",
     "@rollup/plugin-typescript": "8.3.1",
     "@types/multer": "^1.4.5",
     "@typescript-eslint/eslint-plugin": "^5.20.0",
     "@typescript-eslint/parser": "^5.20.0",
     "arg": "^5.0.1",
+    "chai": "~4.3.4",
     "eslint": "^8.13.0",
     "eslint-config-prettier": "^8.5.0",
     "eslint-plugin-prettier": "^4.0.0",
@@ -58,10 +60,15 @@
     "build:win": "tsc --noEmit false --declaration true --emitDeclarationOnly true --outDir dist/types & rollup -c",
     "watch": "rollup -wc",
     "start": "node src/tests/runner.js serveOnly",
-    "test": "NODE_OPTIONS=--inspect node src/tests/runner.js",
-    "test:win": "SET NODE_OPTIONS=--inspect & node src/tests/runner.js",
+    "test": "yarn test:unit && yarn test:browser",
+    "test:browser": "playwright test",
+    "test:unit": "NODE_OPTIONS=--inspect node src/tests/runner.js",
+    "test:unit:win": "SET NODE_OPTIONS=--inspect & node src/tests/runner.js",
     "prerelease": "yarn build && git --no-pager diff && echo && npm pack --dry-run && echo && read -n 1 -p \"Look OK? Press any key to publish and commit v$npm_package_version\" && echo",
     "release": "npm publish && git commit -am \"$npm_package_name v$npm_package_version\" && git push",
     "lint": "eslint . --ext .ts"
+  },
+  "engines": {
+    "node": ">= 14"
   }
 }
diff --git a/playwright.config.ts b/playwright.config.ts
@@ -0,0 +1,27 @@
+import { type PlaywrightTestConfig, devices } from "@playwright/test"
+
+const config: PlaywrightTestConfig = {
+  projects: [
+    {
+      name: "chrome",
+      use: { ...devices["Desktop Chrome"] },
+    },
+    {
+      name: "firefox",
+      use: { ...devices["Desktop Firefox"] },
+    },
+  ],
+  testDir: "./src/tests/functional",
+  testMatch: /.*_tests\.ts/,
+  webServer: {
+    command: "yarn start",
+    url: "http://localhost:9000/src/tests/fixtures/test.js",
+    timeout: 120 * 1000,
+    reuseExistingServer: !process.env.CI,
+  },
+  use: {
+    baseURL: "http://localhost:9000/",
+  },
+}
+
+export default config
diff --git a/rollup.config.js b/rollup.config.js
@@ -30,28 +30,6 @@ export default [
     }
   },
 
-  {
-    input: "src/tests/functional/index.ts",
-    output: [
-      {
-        file: "dist/tests/functional.js",
-        format: "cjs",
-        sourcemap: true
-      }
-    ],
-    plugins: [
-      resolve(),
-      typescript()
-    ],
-    external: [
-      "http",
-      "intern"
-    ],
-    watch: {
-      include: "src/tests/**"
-    }
-  },
-
   {
     input: "src/tests/unit/index.ts",
     output: [

diff --git a/src/core/drive/form_submission.ts b/src/core/drive/form_submission.ts
@@ -1,7 +1,7 @@
 import { FetchRequest, FetchMethod, fetchMethodFromString, FetchRequestHeaders } from "../../http/fetch_request"
 import { FetchResponse } from "../../http/fetch_response"
 import { expandURL } from "../url"
-import { dispatch } from "../../util"
+import { attributeTrue, dispatch } from "../../util"
 import { StreamMessage } from "../streams/stream_message"
 
 export interface FormSubmissionDelegate {
@@ -29,6 +29,11 @@ enum FormEnctype {
   plain = "text/plain",
 }
 
+export type TurboSubmitStartEvent = CustomEvent<{ formSubmission: FormSubmission }>
+export type TurboSubmitEndEvent = CustomEvent<
+  { formSubmission: FormSubmission } & { [K in keyof FormSubmissionResult]?: FormSubmissionResult[K] }
+>
+
 function formEnctypeFromString(encoding: string): FormEnctype {
   switch (encoding.toLowerCase()) {
     case FormEnctype.multipart:
@@ -153,14 +158,17 @@ export class FormSubmission {
       if (token) {
         headers["X-CSRF-Token"] = token
       }
+    }
+
+    if (this.requestAcceptsTurboStreamResponse(request)) {
       headers["Accept"] = [StreamMessage.contentType, headers["Accept"]].join(", ")
     }
   }
 
   requestStarted(_request: FetchRequest) {
     this.state = FormSubmissionState.waiting
     this.submitter?.setAttribute("disabled", "")
-    dispatch("turbo:submit-start", {
+    dispatch<TurboSubmitStartEvent>("turbo:submit-start", {
       target: this.formElement,
       detail: { formSubmission: this },
     })
@@ -197,16 +205,22 @@ export class FormSubmission {
   requestFinished(_request: FetchRequest) {
     this.state = FormSubmissionState.stopped
     this.submitter?.removeAttribute("disabled")
-    dispatch("turbo:submit-end", {
+    dispatch<TurboSubmitEndEvent>("turbo:submit-end", {
       target: this.formElement,
       detail: { formSubmission: this, ...this.result },
     })
     this.delegate.formSubmissionFinished(this)
   }
 
+  // Private
+
   requestMustRedirect(request: FetchRequest) {
     return !request.isIdempotent && this.mustRedirect
   }
+
+  requestAcceptsTurboStreamResponse(request: FetchRequest) {
+    return !request.isIdempotent || attributeTrue(this.formElement, "data-turbo-stream")
+  }
 }
 
 function buildFormData(formElement: HTMLFormElement, submitter?: HTMLElement): FormData {

diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts
@@ -108,9 +108,9 @@ export class Navigator {
     if (responseHTML) {
       const snapshot = PageSnapshot.fromHTMLString(responseHTML)
       if (fetchResponse.serverError) {
-        await this.view.renderError(snapshot)
+        await this.view.renderError(snapshot, this.currentVisit)
       } else {
-        await this.view.renderPage(snapshot)
+        await this.view.renderPage(snapshot, false, true, this.currentVisit)
       }
       this.view.scrollToTop()
       this.view.clearSnapshotCache()

diff --git a/src/core/drive/page_view.ts b/src/core/drive/page_view.ts
@@ -4,6 +4,7 @@ import { ErrorRenderer } from "./error_renderer"
 import { PageRenderer } from "./page_renderer"
 import { PageSnapshot } from "./page_snapshot"
 import { SnapshotCache } from "./snapshot_cache"
+import { Visit } from "./visit"
 
 export interface PageViewDelegate extends ViewDelegate<PageSnapshot> {
   viewWillCacheSnapshot(): void
@@ -16,15 +17,18 @@ export class PageView extends View<Element, PageSnapshot, PageViewRenderer, Page
   lastRenderedLocation = new URL(location.href)
   forceReloaded = false
 
-  renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true) {
+  renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true, visit?: Visit) {
     const renderer = new PageRenderer(this.snapshot, snapshot, isPreview, willRender)
     if (!renderer.shouldRender) {
       this.forceReloaded = true
+    } else {
+      visit?.changeHistory()
     }
     return this.render(renderer)
   }
 
-  renderError(snapshot: PageSnapshot) {
+  renderError(snapshot: PageSnapshot, visit?: Visit) {
+    visit?.changeHistory()
     const renderer = new ErrorRenderer(this.snapshot, snapshot, false)
     return this.render(renderer)
   }

diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -228,11 +228,11 @@ export class Visit implements FetchRequestDelegate {
         this.cacheSnapshot()
         if (this.view.renderPromise) await this.view.renderPromise
         if (isSuccessful(statusCode) && responseHTML != null) {
-          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender)
+          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender, this)
           this.adapter.visitRendered(this)
           this.complete()
         } else {
-          await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML))
+          await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this)
           this.adapter.visitRendered(this)
           this.fail()
         }
@@ -270,7 +270,7 @@ export class Visit implements FetchRequestDelegate {
           this.adapter.visitRendered(this)
         } else {
           if (this.view.renderPromise) await this.view.renderPromise
-          await this.view.renderPage(snapshot, isPreview, this.willRender)
+          await this.view.renderPage(snapshot, isPreview, this.willRender, this)
           this.adapter.visitRendered(this)
           if (!isPreview) {
             this.complete()