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

Improve TypeScript types #301

Merged
merged 2 commits into from
Jun 16, 2022
Merged

Improve TypeScript types #301

merged 2 commits into from
Jun 16, 2022

Conversation

blixt
Copy link
Contributor

@blixt blixt commented Jun 16, 2022

Updating the TypeScript types to include recent additions. Also attempts to accurately represent the types in the registry map, though that could use some extra scrutiny.

index.d.ts Outdated
Comment on lines 173 to 177
/**
* Module type
* @deprecated This field is never set.
*/
t: null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this property could be removed completely because I don't see it being used (there is another t property being returned by the fetch functionality, but that object does not appear to ever overlap with the one set in registry).

index.d.ts Outdated Show resolved Hide resolved
@blixt blixt marked this pull request as ready for review June 16, 2022 11:54
@guybedford
Copy link
Owner

The resolver / importShim types look great. For the registry unfortunately that's a private API (so won't be providing semver-level backwards-compat), so I'd be weary to type it unless TypeScript has some explicit way of indicating something is a private / non semver API.

@blixt
Copy link
Contributor Author

blixt commented Jun 16, 2022

We can exclude the _r property and leave that up to the intrepid developer that needs to use it to type in their private repo (which is what we're doing today at Framer). It would be nice to get some level of long-term support for a reverse lookup of blob URL to import specifier though. The following code is how I'm using the _r property today, maybe it could be exposed in a follow-up PR?

/**
 * Make blob URLs (modules shimmed locally to work with our dynamic import map)
 * human readable again, very useful for debugging purposes.
 */
function replaceBlobURLs(value: string): string {
    const reverseMap = new Map<string, string>()

    // Replace blob URLs with import map entries.
    const importMap = importShim.getImportMap()
    for (const [specifier, url] of Object.entries(importMap.imports)) {
        if (!url.startsWith("blob:")) continue
        reverseMap.set(url, specifier)
    }

    // Replace blob URLs with URL imports.
    for (const [specifier, entry] of Object.entries(importShim._r)) {
        if (!entry.b) continue
        if (reverseMap.has(entry.b)) {
            // Don't override import map entries which should be more accurate.
            continue
        }
        // Sometimes blob URLs point to other blob URLs, so attempt to resolve.
        // TODO: This should probably also follow multi-step chains of blob URLs.
        reverseMap.set(entry.b, reverseMap.get(specifier) ?? specifier)
    }

    return value.replace(/blob:https?:\/\/[^/]+\/[a-z0-9-]+/g, blobURL => reverseMap.get(blobURL) ?? blobURL)
}

Comment on lines +61 to +65
resolve: (
id: string,
parentUrl: string,
resolve: (id: string, parentUrl: string) => string
) => string | Promise<string>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prettier did this but I figured I'd leave it in since it's less likely to cause more diffs in the future (assuming most devs have Prettier running).

Copy link
Owner

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks, yes what you are checking in your code will be stable at least here and likely won't change.

Out of interest, are you not using the native import maps path in Chrome? Surely you get the proper native debugging experience there? Or are you using shim mode entirely?

@blixt
Copy link
Contributor Author

blixt commented Jun 16, 2022

Out of interest, are you not using the native import maps path in Chrome? Surely you get the proper native debugging experience there? Or are you using shim mode entirely?

Because Framer is essentially a code editor itself (though we tend to mostly generate code from visually edited elements), supporting NPM dependencies that can be dynamically added within an active session, and much more, we must use the shim mode entirely indeed.

We update the import maps dynamically hundreds of times in a single edit session, but we still want to output helpful messages to the end-user's console when things do end up going wrong. Since we wrap all code evaluation (which happens within a sandboxed iframe) in our own error handling, we can catch the error message and update it before anything is printed in the developer console.

@guybedford guybedford merged commit 53a767c into guybedford:main Jun 16, 2022
@blixt blixt deleted the improve-types branch June 16, 2022 18:05
@guybedford
Copy link
Owner

Ah makes sense. There may be places with rethrown errors where es-module-shims itself might want to do a reverse lookup. If that becomes the case, we could expose the function internally as well then.

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