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(scripts): deprecate implicit proxy and $script #379

Merged
merged 10 commits into from
Aug 19, 2024

Conversation

harlan-zw
Copy link
Collaborator

@harlan-zw harlan-zw commented Aug 16, 2024

Background

I'm considering one major API change before the official Nuxt Scripts announcement regarding the proxy API, this will be backwards compatible.

The proxy API provides a good DX for end users IMO and it allows good future-proofing when loading scripts using alternative strategies such as web workers.

However, it introduces behaviour that is hard to predict, and there's no way to know what will happen when you call the function universally across all scripts.

For users who don't consider how the code can work, it would be easy to assume they are interacting with the loaded script. When it fails they may have a difficult time debugging as they'll only have a proxy to inspect.

For something like Google Analytics this isn't a big issue, see:

const { gtag } = useScript(...)
// script is not loaded
gtag() // gtag is an async function with a void return
// script is loaded
gtag() // gtag is a regular function with a void return

However, when users try and use the proxy API where there is a return, it can be super fiddly

const { loadUser } = useScript(...)
// script is not loaded
const { firstName } = await loadUser() // loadUser is an async function with a user return - we have to alter the underlying API
// script is loaded
const { firstName } = loadUser() // loadUser is a regular function with a user return - just works

So we're deprecating this behavior completely and only supporting the previous behavior by accessing the proxy value.

const { proxy }= useScript(...)
const { loadUser } = myScript.proxy
// an async function is returned for all properties and will always behave the same regardless of load state
const { firstName } = await loadUser()

We can see this implemented using gtag as:

const { proxy } = useScript(...)
// script is not loaded
proxy.gtag() // gtag is an async function with a void return
// script is loaded
proxy.gtag() // gtag is an async function with a void return

We can easily type this functionality so the user will always know what's going on.

This also allows us to simplify useScript, we can now return the $script instance directly as is as there are no other data returned on the function.

const script = useScript({
 // ...
})
script.then(() => {})
//
script.load(() => {})
// 
script.status // loaded

Technical Details

⚠️ Deprecations

  • $script, you should now access the previous script instance functions at the top level.
  • Proxy functions, you should now access them from the proxy object.

Features

  • Proxy functions now have proper types.
  • Can nest calls within the proxy, for example dataLayer.push() now works without stubbing.
  • New instance on the script instance which holds the reference to the use() output.
  • New proxy on the script instance.
  • Allow access to properties within the third-party script through the proxy API by wrapping them in an async function call.

Copy link
Contributor

@huang-julien huang-julien left a comment

Choose a reason for hiding this comment

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

I'm good with it. This will probably break a few things that is already in place in nuxt scripts tests and playground

@harlan-zw
Copy link
Collaborator Author

I'm good with it. This will probably break a few things that is already in place in nuxt scripts tests and playground

Do you have any opinions though?

This will be implemented as a non breaking change

@huang-julien
Copy link
Contributor

Is it possible to maybe use a build-time flag that is disabled by default (or enabled if you want it non breaking) that would treeshake the old behavior ?

Also in your example :

script.then(() => {})
script.load(() => {})

Is load() signature going to be modified ?

@huang-julien
Copy link
Contributor

huang-julien commented Aug 17, 2024

IMO, removing proxy is probably not hurting too much DX as it can be hard to understand how things works under the hood for beginners. Also because it's hard for us to stub everything in SSR and determine what behavior we should do if anything is done in SSR but missing client side.

If it's for debugging purpose because we're only getting the proxy object, i think it's possible to explore some Object reflection utilities in dev mode to access to the object and know what is happening + add a documentation aboutit .

@harlan-zw
Copy link
Collaborator Author

Thanks, I was a bit on the fence so that's useful 🙂

@huang-julien
Copy link
Contributor

Or we could do a breaking change with the composable directly returning the promise maybe ? To avoid users to use .load().then()

@harlan-zw
Copy link
Collaborator Author

harlan-zw commented Aug 17, 2024

Or we could do a breaking change with the composable directly returning the promise maybe ? To avoid users to use .load().then()

yes anything previously on $script will be top level now, including the promise, it's not a breaking change though, you could always just call then()

@harlan-zw harlan-zw marked this pull request as ready for review August 18, 2024 12:58
@huang-julien
Copy link
Contributor

One thing i didn't think about now that the promise is to level. I don't know what impact this will have on users since a lot will probably be confused with typescript type being a Promise and not being allowed to use await in a server side context.

@harlan-zw
Copy link
Collaborator Author

harlan-zw commented Aug 19, 2024

One thing i didn't think about now that the promise is to level. I don't know what impact this will have on users since a lot will probably be confused with typescript type being a Promise and not being allowed to use await in a server side context.

Yes, this has always been an issue nuxt/scripts#46, the only difference is it can also be top-level now. The load() function has the same issue.

I'll investigate some runtime solutions on this

@harlan-zw harlan-zw changed the title feat(scripts): drop implicit proxying feat(scripts): deprecate implicit proxying Aug 19, 2024
@harlan-zw harlan-zw changed the title feat(scripts): deprecate implicit proxying feat(scripts): flatten API, deprecate implicitness Aug 19, 2024
@harlan-zw harlan-zw changed the title feat(scripts): flatten API, deprecate implicitness feat(scripts): deprecate implicit proxy and $script Aug 19, 2024
@harlan-zw harlan-zw merged commit 3e64bde into main Aug 19, 2024
2 checks passed
@harlan-zw harlan-zw deleted the feat/scripts-drop-implicit-proxying branch August 19, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants