Skip to content

Use correct CSS version when injecting link tag #187

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

Merged
merged 5 commits into from
May 27, 2023
Merged

Use correct CSS version when injecting link tag #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2573a54
refactor: Prefer disabled over enabled
razor-x May 27, 2023
9ec5813
refactor: Return early from effect
razor-x May 27, 2023
12ddff0
docs: Import CSS in example
razor-x May 27, 2023
c7c88e8
feat: Add useUnminifiedCss prop
razor-x May 27, 2023
12172b3
fix: Use correct version for stylesheet
razor-x May 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/basic/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export const App = (): JSX.Element => {
endpoint={import.meta.env.SEAM_ENDPOINT}
publishableKey={import.meta.env.SEAM_PUBLISHABLE_KEY}
userIdentifierKey={import.meta.env.SEAM_USER_IDENTIFIER_KEY}
disableCssInjection
Copy link
Member Author

Choose a reason for hiding this comment

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

We have to put this in the example since when running locally the example should use the latest styles. We can keep the font injection.

>
<main>
<h1>Seam Components</h1>
Expand Down
2 changes: 2 additions & 0 deletions examples/basic/src/main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import '@seamapi/react/index.css'

import React from 'react'
import { createRoot } from 'react-dom/client'

Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"lint": "eslint --ignore-path .gitignore --ext .js,.cjs,.mjs,.ts,.tsx .",
"prelint": "prettier --check --ignore-path .gitignore .",
"postlint": "stylelint '**/*.scss'",
"prepack": "echo \"export default '$(jq -r .version < package.json)'\" > lib/version.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

Normally I would like to use a platform independent script (can always be done later), but this will ONLY run on CI so it should be safe to do this for now.

"postversion": "git push --follow-tags",
"storybook": "storybook dev --port 6006",
"storybook:docs": "storybook dev --docs --port 6007",
Expand Down
27 changes: 17 additions & 10 deletions src/lib/SeamProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,12 @@ export interface SeamContext {
clientSessionToken?: string | undefined
}

type SeamProviderProps = {
disableCssInjection?: boolean | undefined
disableFontInjection?: boolean | undefined
} & (
| SeamProviderPropsWithClient
| (SeamProviderPropsWithPublishableKey & AllowedSeamClientOptions)
| (SeamProviderPropsWithClientSessionToken & AllowedSeamClientOptions)
)
type SeamProviderProps = SeamProviderBaseProps &
(
| SeamProviderPropsWithClient
| (SeamProviderPropsWithPublishableKey & AllowedSeamClientOptions)
| (SeamProviderPropsWithClientSessionToken & AllowedSeamClientOptions)
)

interface SeamProviderPropsWithClient {
client: Seam
Expand All @@ -47,10 +45,19 @@ interface SeamProviderPropsWithClientSessionToken {
clientSessionToken: string
}

interface SeamProviderBaseProps {
disableCssInjection?: boolean | undefined
disableFontInjection?: boolean | undefined
useUnminifiedCss?: boolean | undefined
}

type AllowedSeamClientOptions = Pick<SeamClientOptions, 'endpoint'>

export function SeamProvider({
children,
disableCssInjection = false,
disableFontInjection = false,
useUnminifiedCss = false,
...props
}: PropsWithChildren<SeamProviderProps>): JSX.Element {
const { Provider } = seamContext
Expand All @@ -76,8 +83,8 @@ export function SeamProvider({
)
}

useSeamStyles({ enabled: !(props.disableCssInjection ?? false) })
useSeamFont({ enabled: !(props.disableFontInjection ?? false) })
useSeamStyles({ disabled: disableCssInjection, unminified: useUnminifiedCss })
useSeamFont({ disabled: disableFontInjection })

return (
<div className='seam-components'>
Expand Down
21 changes: 10 additions & 11 deletions src/lib/seam/use-seam-font.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,18 @@ import { useEffect } from 'react'
const fontUrl =
'https://fonts.googleapis.com/css2?family=Source+Sans+Pro:wght@400;600;700&display=swap'

export const useSeamFont = ({ enabled }: { enabled: boolean }) => {
export const useSeamFont = ({ disabled = false }: { disabled?: boolean }) => {
useEffect(() => {
if (disabled) return
if (globalThis.document == null) return
if (!enabled) return
const linkEl = globalThis.document.querySelector(`link[href="${fontUrl}"]`)

if (linkEl === null) {
const link = globalThis.document.createElement('link')
link.rel = 'stylesheet'
link.type = 'text/css'
link.href = fontUrl
const linkEl = globalThis.document.querySelector(`link[href="${fontUrl}"]`)
if (linkEl != null) return

globalThis.document.head.appendChild(link)
}
}, [enabled])
const link = globalThis.document.createElement('link')
link.rel = 'stylesheet'
link.type = 'text/css'
link.href = fontUrl
globalThis.document.head.appendChild(link)
}, [disabled])
}
36 changes: 23 additions & 13 deletions src/lib/seam/use-seam-styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,31 @@
import { useEffect } from 'react'

// TODO figure out the version automatically
const cssUrl = 'https://www.unpkg.com/@seamapi/react@1.1.0/index.min.css'
import version from 'lib/version.js'

const origin = 'https://react.seam.co'
Copy link
Contributor

Choose a reason for hiding this comment

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

If we always use the css file in the release / bundle, wouldn't it always have the correct version?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because we are injecting a URL that contains the version number which must match the version of the package that is being used by the user. I'm not sure if that explanation is more clear, but it's correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

No because we are injecting a URL that contains the version number

Yep, was wondering what the benefits of injecting via URL vs. just including what is bundled with the package are?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, there is a pretty long internal discussion about this. The short version is that having it inject the fonts and the styles simplifies adoption and removes all the extra steps you need to use this.

It's still possible to opt out of the HTML injection and do it the other way for users who prefer that.

Copy link
Contributor

@mikewuu mikewuu May 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Was actually thinking of dynamically importing the css via relative paths, but of course that wouldn't work because this is runtime.

What if we always include the CSS, but just change the class name? Won't have to deal with version numbers, and no dynamic loading necessary?

Root component / provider

const class = disableStyles ? '' : 'seam-components'

return (<div className={class}>...</div>)

Copy link
Member Author

Choose a reason for hiding this comment

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

What if we always include the CSS

I'm not sure I understand this. With this update we are always including the css unless the user opts out. I'd prefer not to inject the via an inline style tag since that bloats the js bundle size and cannot take advantage of caching the same way (among other things).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to inject the via an inline style tag

Yep, I mean we'd be able to avoid that. Here's a PR:

#191


export const useSeamStyles = ({
disabled = false,
unminified = false,
}: {
unminified?: boolean
disabled?: boolean
}) => {
const ext = `${unminified ? '' : 'min.'}css`
const cssUrl = `${origin}/v/${version ?? ''}/index.${ext}`

export const useSeamStyles = ({ enabled }: { enabled: boolean }) => {
useEffect(() => {
if (version === null) return
Copy link
Member Author

Choose a reason for hiding this comment

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

In development the version will always be null.

if (disabled) return
if (globalThis.document == null) return
if (!enabled) return
const linkEl = globalThis.document.querySelector(`link[href="${cssUrl}"]`)

if (linkEl === null) {
const link = globalThis.document.createElement('link')
link.rel = 'stylesheet'
link.type = 'text/css'
link.href = cssUrl
const linkEl = globalThis.document.querySelector(`link[href="${cssUrl}"]`)
if (linkEl != null) return

globalThis.document.head.appendChild(link)
}
}, [enabled])
const link = globalThis.document.createElement('link')
link.rel = 'stylesheet'
link.type = 'text/css'
link.href = cssUrl
globalThis.document.head.appendChild(link)
}, [disabled, cssUrl])
}
1 change: 1 addition & 0 deletions src/lib/version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default null