Skip to content
This repository has been archived by the owner on Oct 7, 2022. It is now read-only.

Add client side validation #131

Merged
merged 10 commits into from
Sep 17, 2021
Merged

Add client side validation #131

merged 10 commits into from
Sep 17, 2021

Conversation

Carla-Moz
Copy link
Contributor

closes #99

@rhelmer
Copy link
Contributor

rhelmer commented Sep 15, 2021

The current failure in CI is:

vite v2.5.7 building SSR bundle for production...
transforming (379) node_modules/@protobufjs/codegen/index.jsUse of eval is strongly discouraged, as it poses security risks and may cause issues with minification
4:54:35 PM [vite-plugin-svelte] The following packages did not export their `package.json` file so we could not check the "svelte" field. If you had difficulties importing svelte components from a package, then please contact the author and ask them to export the package.json file.
- @firebase/firestore
✓ 411 modules transformed.
.svelte-kit/output/server/app.js   1558.98 KiB

Run npm run preview to preview your production build locally.

> Using @sveltejs/adapter-static
> The requested module 'tslib' does not provide an export named 'default'
file:///Users/mozilla/src/rally-web-platform/.svelte-kit/output/server/app.js:23
import require$$0$4, { __extends, __assign, __values, __read, __awaiter, __generator, __spreadArray, __rest } from "tslib";
       ^^^^^^^^^^^^
SyntaxError: The requested module 'tslib' does not provide an export named 'default'
    at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21)
    at async ModuleJob.run (internal/modules/esm/module_job.js:143:5)
    at async Loader.import (internal/modules/esm/loader.js:165:24)
    at async prerender (file:///Users/mozilla/src/rally-web-platform/node_modules/@sveltejs/kit/dist/chunks/index6.js:113:14)
    at async Object.prerender (file:///Users/mozilla/src/rally-web-platform/node_modules/@sveltejs/kit/dist/chunks/index6.js:339:5)
    at async adapt (file:///Users/mozilla/src/rally-web-platform/node_modules/@sveltejs/adapter-static/index.js:13:4)
    at async adapt (file:///Users/mozilla/src/rally-web-platform/node_modules/@sveltejs/kit/dist/chunks/index6.js:365:2)
    at async file:///Users/mozilla/src/rally-web-platform/node_modules/@sveltejs/kit/dist/cli.js:933:5

On the surface, the problem here seems to be that the es version of the tslib module isn't being imported for some reason.

That's worth looking into, but @Carla-Moz note that I seem to be able to make the error go away if I revert the changes in package-lock.json and then npm run install && npm run build.

Do you want to give that a try and see if it unblocks you? We should also go through and make sure we're on the latest version of our devDependencies like sveltekit/tslib/etc. since there is some reason this is happening. I'm pretty sure I've seen the above error before too.

@Carla-Moz
Copy link
Contributor Author

@rhelmer, Thank you! Yes, that removed the error for me. And yes, let's make sure our devDependencies are all on the latest version.

@Carla-Moz Carla-Moz self-assigned this Sep 16, 2021
@Carla-Moz Carla-Moz requested a review from rhelmer September 16, 2021 13:05
Copy link
Contributor

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

This is a big improvement overall :) I made a few suggestions that you can take or leave, the only one I feel strongly about is I found by pasting in passwords, or using Firefox's auto-suggest password feature:

image
image

Note how the buttons are disabled even though a valid password, if I paste in the password it with the keyboard it works fine. I suspect it's because the button state is changed in handleKeyup and that event doesn't fire in this case. If I right-click and paste with the mouse I see the same behavior, buttons stay disabled.

Related to button state, I noticed the buttons are not disabled even though I'm getting the "password too short" warning:
image

src/app.html Show resolved Hide resolved
src/routes/signup/index.svelte Outdated Show resolved Hide resolved
src/routes/signup/index.svelte Outdated Show resolved Hide resolved
@Carla-Moz Carla-Moz merged commit 5291414 into master Sep 17, 2021
rhelmer pushed a commit that referenced this pull request Sep 17, 2021
* added basic validation

* returned form ids to original names

* reverted to old package-lock.json

* updated form

* addressed changes

* addressed failed tests
@Carla-Moz Carla-Moz deleted the add-client-side-validation branch November 29, 2021 21:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task of #117: Add client-side form validation
2 participants