Respect existing sourceURL when both sourceURL and sourceMappingURL exists #372

lewisl9029 · 2023-05-12T03:05:09Z

Hey there! I started making use of sourceURL comments to preserve breakpoints across updates in Reflame, and discovered that shim mode seems to ignore the existing sourceURL and adds its own if both sourceURL and sourceMappingURL exists.

I've captured this behavior in a new test. Here's the existing behavior:

(note the 2 sourceURLs)

Here's the new behavior after my fix:

A few things contributed to the existing behavior:

The regex we use matches for end of string, so will never actually match a sourceURL comment that's not at the last line
resolvedSource.replace only executes once and only replaces the first instance, so it could never properly handle the case where both exist.

My fix splits the previous regex into 2, one for sourceURL and one for sourceMappingURL, and performs 2 replace passes to handle both independently.

Not sure how significant this replacement pass is to the overall perf budget. If it is significant enough, it could be worth trying to optimize this a bit further?

Happy to iterate on this as needed. Cheers!

guybedford · 2023-05-14T20:16:22Z

This looks great. Yes I would prefer to avoid the double regex if possible, if there's a way to do that, as performance is very important here.

One option could be to use a search instead of a replace, and then do the logic via splicing. Possibly replacing the existing sourceURL line addition to share the same logic?

lewisl9029 · 2023-05-15T02:38:01Z

Sounds good. Though I'm curious if regexes are even necessary here?

Is there anything stopping us from just doing resolvedSource.lastIndexOf('\n//# sourceURL=') and then treating everything until the next \n or end of file as the url to replace? That would likely perform much better than a regex search right? Wondering what I might be missing here.

guybedford · 2023-05-15T03:15:20Z

@lewisl9029 we can certainly move away from using a regex sure, as long as we retain the remaining functionality. History in #229 and #122.

lewisl9029 · 2023-05-15T04:34:56Z

@guybedford I pushed an update that uses lastIndexOf to search from the back using hard coded strings instead of regexes. Tests seem to be passing on my machine so I think behavior has been preserved.

In terms of performance, it still does 2 passes through the string, but my gut says there's a chance this could still be faster than the current implementation that uses a more costly regex replace and searches from the front.

I was trying to verify this using chomp bench, but it seems to just open the browser and then gets stuck, even when I check out the main branch. Any ideas how I should be benchmarking this?

guybedford · 2023-05-21T19:54:29Z

@lewisl9029 I'm not sure if it's actually better, but I've pushed up a variation that only does additive splicing based on the existing index tracking.

Hypothetical benefits are:

Since string splicing is only additive, there's in theory no need to recopy the string or deal with ropes (whether this is actually compatible with v8 I don't know).
Any sourceURLs before import or export statements will be naturally ignored

It may be better to stick with your approach actually, but thought I'd take a look anyway. Let me know your thoughts :)

lewisl9029 · 2023-05-23T18:56:28Z

Hey @guybedford, just gave this new version (published at https://www.npmjs.com/package/@lewisl9029/es-module-shims/v/1.7.2-4) a try in a customer app and found a potential bug. You can see it in this preview.

Illegal return statement (at InternalRouter.tsx:77:5)

Seems like it's inserting the comments in the middle of the file? I haven't had a chance to dive in and debug yet, but here's some info that might help:

Actual output:

import { jsxDEV as _jsxDEV } from /*"react/jsx-dev-runtime"*/'blob:https://highlight-test-lewisl.reflame.dev/f39d1c4a-449a-46a5-a400-db19a6b143db';
let $reflamePathname = new URL(importShim._r['https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw'].m.url).pathname, $reflamePreviousRefreshReg = self.$RefreshReg$, $reflamePreviousRefreshSig = self.$RefreshSig$;
self.$RefreshReg$ = (type, id)=>{
    let fullId = $reflamePathname + ` ${id}`;
    self.$reflame.reactRefreshRuntime.register(type, fullId);
}, self.$RefreshSig$ = self.$reflame.reactRefreshRuntime.createSignatureFunctionForTransform;
var _c, _c2, _c4, _s = $RefreshSig$();
import { AppLoadingState, useAppLoadingContext } from /*'@context/AppLoadingContext'*/'blob:https://highlight-test-lewisl.reflame.dev/c27502d6-3303-434b-a899-2f6338a16b32';
import { lazy, Suspense, useEffect } from /*'react'*/'blob:https://highlight-test-lewisl.reflame.dev/7ebe3cb0-0362-4838-a084-007283cb15ca';
import { Route, Routes } from /*'react-router-dom'*/'blob:https://highlight-test-lewisl.reflame.dev/936df7cf-a117-4ae6-bf1b-9c0ab5842cbe';
let QueryBuilderPage = lazy(_c = ()=>importShim('../../pages/Internal/QueryBuilderPage', 'https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw')), OpenSearchQueryPage = lazy(_c2 = ()=>importShim('../../pages/Internal/OpenSearchQueryPage', 'https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw')), InternalPage = lazy(_c4 = ()=>importShim('../../pages/Internal/InternalPage')), InternalRouter = ()=>{
    _s();
    let { setLoadingState  } = useAppLoadingContext();
    return useEffect(()=>{
        setLoadingState(AppLoadingState.LOADED);
    }, [
        setLoadingState
    ]), _jsxDEV(Routes, {
        children: [
            _jsxDEV(Route, {
                path: "query-builder",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(QueryBuilderPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 25,
                columnNumber: 4
            }, this),
            _jsxDEV(Route, {
                path: "opensearch-query-builder",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(OpenSearchQueryPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 33,
                columnNumber: 4
            }, this),
            _jsxDEV(Route, {
                path: "*",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(InternalPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 41,
                columnNumber: 4
            }, this)
        ]
    }, void 0, !0, {
        fileName: "InternalRouter.tsx",
        lineNumber: 24,
        columnNumber: 3
    }, this);
};
_s(InternalRouter, "MjRUeZTIsJkLtQSMIIfkz1O91R8=", !1, function() {
    return [
        useAppLoadingContext
    ];
});
export default InternalRouter;
$RefreshReg$(_c, "QueryBuilderPage$lazy"), $RefreshReg$(QueryBuilderPage, "QueryBuilderPage"), $RefreshReg$(_c2, "OpenSearchQueryPage$lazy"), $RefreshReg$(OpenSearchQueryPage, "OpenSearchQueryPage"), $RefreshReg$(_c4, "InternalPage$lazy"), $RefreshReg$(InternalPage, "InternalPage"), $RefreshReg$(InternalRouter, "InternalRouter"), self.$RefreshReg$ = $reflamePreviousRefreshReg, self.$RefreshSig$ = $reflamePreviousRefreshSig, self.$reflame.registerAcceptCallback({
    pathname: $reflamePathname,
    callback: ({ pathname , id  })=>{
        id ? console.debug("accepting", pathname, "to", id) : console.debug("accepting", pathname), self.$reflame.performReactRefresh();
    }
});

//# sourceURL=https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx
//# sourceMappingURL=https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx.map?~r_rid=67jggMiLfkXK7qNjSE3Yq8gq498, 'https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw')), InternalRouter = ()=>{
    _s();
    let { setLoadingState  } = useAppLoadingContext();
    return useEffect(()=>{
        setLoadingState(AppLoadingState.LOADED);
    }, [
        setLoadingState
    ]), _jsxDEV(Routes, {
        children: [
            _jsxDEV(Route, {
                path: "query-builder",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(QueryBuilderPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 25,
                columnNumber: 4
            }, this),
            _jsxDEV(Route, {
                path: "opensearch-query-builder",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(OpenSearchQueryPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 33,
                columnNumber: 4
            }, this),
            _jsxDEV(Route, {
                path: "*",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(InternalPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 41,
                columnNumber: 4
            }, this)
        ]
    }, void 0, !0, {
        fileName: "InternalRouter.tsx",
        lineNumber: 24,
        columnNumber: 3
    }, this);
};
_s(InternalRouter, "MjRUeZTIsJkLtQSMIIfkz1O91R8=", !1, function() {
    return [
        useAppLoadingContext
    ];
});
export default InternalRouter;
$RefreshReg$(_c, "QueryBuilderPage$lazy"), $RefreshReg$(QueryBuilderPage, "QueryBuilderPage"), $RefreshReg$(_c2, "OpenSearchQueryPage$lazy"), $RefreshReg$(OpenSearchQueryPage, "OpenSearchQueryPage"), $RefreshReg$(_c4, "InternalPage$lazy"), $RefreshReg$(InternalPage, "InternalPage"), $RefreshReg$(InternalRouter, "InternalRouter"), self.$RefreshReg$ = $reflamePreviousRefreshReg, self.$RefreshSig$ = $reflamePreviousRefreshSig, self.$reflame.registerAcceptCallback({
    pathname: $reflamePathname,
    callback: ({ pathname , id  })=>{
        id ? console.debug("accepting", pathname, "to", id) : console.debug("accepting", pathname), self.$reflame.performReactRefresh();
    }
});

//# sourceURL=/~r/app/routers/InternalRouter/InternalRouter.tsx
//# sourceMappingURL=InternalRouter.tsx.map?~r_rid=67jggMiLfkXK7qNjSE3Yq8gq498

Expected output (from this preview, using the previous impl published at https://www.npmjs.com/package/@lewisl9029/es-module-shims/v/1.7.2-2):

import { jsxDEV as _jsxDEV } from /*"react/jsx-dev-runtime"*/'blob:https://highlight-test-lewisl.reflame.dev/d971fdc2-90d1-45ce-a04f-8746cd3bb661';
let $reflamePathname = new URL(importShim._r['https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw'].m.url).pathname, $reflamePreviousRefreshReg = self.$RefreshReg$, $reflamePreviousRefreshSig = self.$RefreshSig$;
self.$RefreshReg$ = (type, id)=>{
    let fullId = $reflamePathname + ` ${id}`;
    self.$reflame.reactRefreshRuntime.register(type, fullId);
}, self.$RefreshSig$ = self.$reflame.reactRefreshRuntime.createSignatureFunctionForTransform;
var _c, _c2, _c4, _s = $RefreshSig$();
import { AppLoadingState, useAppLoadingContext } from /*'@context/AppLoadingContext'*/'blob:https://highlight-test-lewisl.reflame.dev/a9019110-1efd-48a1-8ee3-2e3892d091b7';
import { lazy, Suspense, useEffect } from /*'react'*/'blob:https://highlight-test-lewisl.reflame.dev/811305a6-2c7a-4888-8f88-7f78b734ce87';
import { Route, Routes } from /*'react-router-dom'*/'blob:https://highlight-test-lewisl.reflame.dev/2f65abe0-3144-486a-a1de-590d10f77c5d';
let QueryBuilderPage = lazy(_c = ()=>importShim('../../pages/Internal/QueryBuilderPage', 'https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw')), OpenSearchQueryPage = lazy(_c2 = ()=>importShim('../../pages/Internal/OpenSearchQueryPage', 'https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw')), InternalPage = lazy(_c4 = ()=>importShim('../../pages/Internal/InternalPage', 'https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx?~r_rid=t6YVb4pL55zVMEh1ZdlAwV1HYLw')), InternalRouter = ()=>{
    _s();
    let { setLoadingState  } = useAppLoadingContext();
    return useEffect(()=>{
        setLoadingState(AppLoadingState.LOADED);
    }, [
        setLoadingState
    ]), _jsxDEV(Routes, {
        children: [
            _jsxDEV(Route, {
                path: "query-builder",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(QueryBuilderPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 25,
                columnNumber: 4
            }, this),
            _jsxDEV(Route, {
                path: "opensearch-query-builder",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(OpenSearchQueryPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 33,
                columnNumber: 4
            }, this),
            _jsxDEV(Route, {
                path: "*",
                element: _jsxDEV(Suspense, {
                    fallback: null,
                    children: _jsxDEV(InternalPage, {}, void 0, !1, void 0, void 0)
                }, void 0, !1, void 0, void 0)
            }, void 0, !1, {
                fileName: "InternalRouter.tsx",
                lineNumber: 41,
                columnNumber: 4
            }, this)
        ]
    }, void 0, !0, {
        fileName: "InternalRouter.tsx",
        lineNumber: 24,
        columnNumber: 3
    }, this);
};
_s(InternalRouter, "MjRUeZTIsJkLtQSMIIfkz1O91R8=", !1, function() {
    return [
        useAppLoadingContext
    ];
});
export default InternalRouter;
$RefreshReg$(_c, "QueryBuilderPage$lazy"), $RefreshReg$(QueryBuilderPage, "QueryBuilderPage"), $RefreshReg$(_c2, "OpenSearchQueryPage$lazy"), $RefreshReg$(OpenSearchQueryPage, "OpenSearchQueryPage"), $RefreshReg$(_c4, "InternalPage$lazy"), $RefreshReg$(InternalPage, "InternalPage"), $RefreshReg$(InternalRouter, "InternalRouter"), self.$RefreshReg$ = $reflamePreviousRefreshReg, self.$RefreshSig$ = $reflamePreviousRefreshSig, self.$reflame.registerAcceptCallback({
    pathname: $reflamePathname,
    callback: ({ pathname , id  })=>{
        id ? console.debug("accepting", pathname, "to", id) : console.debug("accepting", pathname), self.$reflame.performReactRefresh();
    }
});

//# sourceURL=https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx
//# sourceMappingURL=https://highlight-test-lewisl.reflame.dev/~r/app/routers/InternalRouter/InternalRouter.tsx.map?~r_rid=67jggMiLfkXK7qNjSE3Yq8gq498

guybedford · 2023-05-28T21:25:39Z

Thanks @lewisl9029 that was a bug with the dynamic import handling.

guybedford · 2023-05-28T21:25:49Z

Should be working in the latest commit now.

lewisl9029 · 2023-05-28T22:28:04Z

New version looks great! Just published it at https://www.npmjs.com/package/@lewisl9029/es-module-shims/v/1.7.2-5, and recorded some profiles:

profiles.zip

There's a lot of variance, but roughly speaking it looks like the additive slicing approach ends up being a good 1s faster than the lastIndexOf version from before. :)

guybedford · 2023-05-28T22:56:29Z

Thanks @lewisl9029 for the update, released in 1.7.3.

lewisl9029 added 2 commits May 11, 2023 18:49

Add failing test

708952e

Fix by separating sourceURL replacement

14cef60

lewisl9029 marked this pull request as ready for review May 12, 2023 03:06

Use lastIndexOf instead of regex search

db4ec3a

additive splicing

1bce7c7

fixup dynamic import case

cafa4db

guybedford merged commit 4a6a305 into guybedford:main May 28, 2023
7 checks passed

lewisl9029 deleted the respect-existing-source-url branch May 28, 2023 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect existing sourceURL when both sourceURL and sourceMappingURL exists #372

Respect existing sourceURL when both sourceURL and sourceMappingURL exists #372

lewisl9029 commented May 12, 2023 •

edited

guybedford commented May 14, 2023

lewisl9029 commented May 15, 2023

guybedford commented May 15, 2023

lewisl9029 commented May 15, 2023 •

edited

guybedford commented May 21, 2023

lewisl9029 commented May 23, 2023

guybedford commented May 28, 2023

guybedford commented May 28, 2023

lewisl9029 commented May 28, 2023

guybedford commented May 28, 2023

Respect existing sourceURL when both sourceURL and sourceMappingURL exists #372

Respect existing sourceURL when both sourceURL and sourceMappingURL exists #372

Conversation

lewisl9029 commented May 12, 2023 • edited

guybedford commented May 14, 2023

lewisl9029 commented May 15, 2023

guybedford commented May 15, 2023

lewisl9029 commented May 15, 2023 • edited

guybedford commented May 21, 2023

lewisl9029 commented May 23, 2023

guybedford commented May 28, 2023

guybedford commented May 28, 2023

lewisl9029 commented May 28, 2023

guybedford commented May 28, 2023

lewisl9029 commented May 12, 2023 •

edited

lewisl9029 commented May 15, 2023 •

edited