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

Use of eval and local variables #95

Open
MartinSherburn opened this issue Jun 16, 2020 · 13 comments
Open

Use of eval and local variables #95

MartinSherburn opened this issue Jun 16, 2020 · 13 comments

Comments

@MartinSherburn
Copy link

Some JavaScript engines (specifically Hermes and QuickJS) don't support accessing local variables from within an eval function call. For example:

const foo = "Hello World!"
eval('console.log(foo)');

Throws an exception: Property 'foo' doesn't exist.

This is problematic because nollup makes use of this feature here.

So my question is, why use eval here? I was able to change the code to write it inline without converting it into a string and using eval with minimal changes. But I had to exclude the source maps. It should be feasible to write a single source map file for the whole bundle like rollup does instead of a separate maps for each module. I'm sure there is a reason for this approach but I'd like to understand what it is. For HMR we clearly do need to use eval but from my experiments it doesn't need access to any local variables.

I think potentially using a single source map for the entire bundle could be more efficient, I've heard that for large projects Chrome Dev Tools can struggle with a large quantity of source maps and it makes the client unresponsive when first connecting. I also believe that using eval makes it slower to execute the bundle although I haven't yet gathered any data to back that up.

@PepsRyuu
Copy link
Owner

Hey @MartinSherburn, thanks for the question!

The problem with generating a single source map isn't a technical constraint, but rather a performance one. Generating a single source map for an entire bundle for every file save can be costly from what I've seen so far. It would require collapsing source maps one by one. Using eval allows source maps to be built independently of each other and cached without any need to do any collapsing.

As for local variables, as far as I can tell, local variable access will more or less always be needed. Modules will need access to require and module in some way in order to enable HMR, and same story for imports. I might be misunderstanding something from your question, but I can't see how an eval would work without local variable access. If you've sample code snippets, feel free to share

Hope this helps clarify things! Out of curiousity though, what is the use case for running outside of NodeJS? :)

@MartinSherburn
Copy link
Author

Thanks for the quick and thorough reply @PepsRyuu. I agree that generating a single source map for the entire bundle on every file save would be a problem. But I don't think you need to do that for HMR to work. What I am suggesting is:

  • Generate full output bundle with single source map on first load only.
  • When a file is saved only generate the source maps for the changed modules. Generating the full bundle again isn't necessary.

Regarding eval and HMR. Suppose I have the module:

import dowork from './dowork.js';

dowork(1);

These are the changes that are currently generated when the file is saved:

  changes: [
    {
      id: 0,
      code: 'function (__c__, __r__, __d__, __e__) {\n' +
        '            var _i0\n' +
        '\n' +
        '            var dowork\n' +
        '\n' +
        '            \n' +
        '\n' +
        '            __d__(function () {\n' +
        '                dowork = _i0().default\n' +
        '                \n' +
        '            }, function (require, module, __nollup__global__) {\n' +
        '                "use strict";\n' +
        "                eval('\\n\\\n" +
        'dowork(1);\\\n' +
        "//# sourceMappingURL=...;\n" +
        '                \n' +
        '            });\n' +
        '\n' +
        '            _i0 = __c__(1) && function () { return __r__(1) }\n' +
        '        }'
    }
  ]

And what I am proposing is generating this instead:

  changes: [
    {
      id: 0,
      code: 'function (__c__, __r__, __d__, __e__) {\n' +
        '            var _i0\n' +
        '\n' +
        '            var dowork\n' +
        '\n' +
        '            \n' +
        '\n' +
        '            __d__(function () {\n' +
        '                dowork = _i0().default\n' +
        '                \n' +
        '            }, function (require, module, __nollup__global__) {\n' +
        '                "use strict";\n' +
        '                \n' +
        'dowork(1);\n' +
        '                \n' +
        '            });\n' +
        '\n' +
        '            _i0 = __c__(1) && function () { return __r__(1) }\n' +
        '        }\n' +
        "//# sourceMappingURL=...;"
    }
  ]

I have removed the call to eval which depends on being able to capture the local function dowork. We still need to eval changes.code but that doesn't rely on using any local variables. I have tested this and it works, the part I am missing is actually generating valid //# sourceMappingURL comment at the bottom.

@PepsRyuu
Copy link
Owner

The problem there is that generating a full bundle again is needed. The assumption being made there is that HMR will always be perfect and be used everywhere, and while in theory this would be ideal, it's not applicable for all scenarios.

Two frequent scenarios come to mind:

  • Modules and files that create side-effects and aren't properly cleaned up due to exceptions being thrown or just haven't been written to be cleaned up on a replacement. While HMR can be pretty good at fixing itself, it can still fail during heavy development.

  • Hot Reload which relies on a page refresh. Not all frameworks offer HMR functionality to replace code on the component level, so as a workaround, devs may use a page refresh to automatically refresh the page on a save change.

In those situations, only bundling at the beginning would mean that your code is out of sync with the generated bundle on a page refresh. It would introduce delays of several seconds for very common use cases. eval as far as I know is the only way to solve this problem and keep a fast dev experience.

@MartinSherburn
Copy link
Author

Right, yes indeed there are cases where generating a full bundle is required. One thought here would be to only re-generate the bundle at the point where it is requested by (i.e. on HTTP GET request). Then you only pay the cost of regenerating the full bundle when HMR fails and it has to fall back to a full reload.

But I understand your concerns about frameworks that don't have any HMR at all.

@PepsRyuu
Copy link
Owner

Lazy loading at point of request would work for HMR users, but would mean a page refresh would take several seconds for both HMR and Hot Reload users. Wouldn't be a great developer experience and would likely result in several complaints from users.

It might be worthwhile posting an issue on those JavaScript engine projects and see what their position on eval accessing local variables is. It is part of the JS spec that eval can access local variables, and this issue would likely affect other tools as well. Probably best to target the root of the issue rather than workaround it. :)

@MartinSherburn
Copy link
Author

Thanks for the discussion @PepsRyuu, this was useful. I have already raised the issue with the Hermes developer as well.

@charlag
Copy link
Contributor

charlag commented Dec 2, 2020

Trying to debug some code with WebStorm, which doesn't let you to put breakpoints in "virtual" (nollup://) files as far as I know I stumbled upon this and I have a question: if it's hard to combine source maps, then why combine multiple files into one? Why not just keep the same number of files as source? Is it loading performance? In case of multiple files and non-inline source-maps they could be lazy-generated as well but it's not necessary.

@PepsRyuu
Copy link
Owner

PepsRyuu commented Dec 2, 2020

You could theoretically bundle each file into their own file (like how dynamic imports code split), the code won't be the same though as in the editor (modules will be wrapped and other various compilation like JSX) so sourcemaps are still important, and also there will be a penalty performance because of the network bottleneck. I don't use WebStorm, but it seems strange that it doesn't support the same source maps as Chrome does. I assume Webpack/Parcel have the same problem? They use the same techniques.

Personally I just recommend using the Chrome debugger, it's always going to be the debugger with the most capable and reliable features compared to third party tools.

@charlag
Copy link
Contributor

charlag commented Dec 2, 2020

Thanks for the answer! Network might be a bottleneck indeed but not too much locally I think.
Sourcemaps are indeed still needed but if you need you can still debug compiled code.
WebStorm does support all the same source maps, it's just nollup uses it's own source root for sourceMaps and not some relative location so if you put breakpoint in the file in IDE it's not the same place as "nollup://" location:

Screenshot from 2020-12-02 14-57-52

@charlag
Copy link
Contributor

charlag commented Dec 7, 2020

Another use case where it doesn't work so well is stacktraces. Unless you use source-map-support, your line numbers are only as precise as the start of eval
Screenshot from 2020-12-07 13-37-44

I think that hot-reloading workflow which is one of the best-selling features of nollup does imply that you don't reload the page all the time so I think that initial loading of many files vs one is not a big problem and can be improved with either HTTP/2 or service worker (the latter would mean more work of course and I'm not asking for this, just outlining the options).

@PepsRyuu
Copy link
Owner

PepsRyuu commented Dec 7, 2020

I was actually investigating earlier how to go about file splitting while maintaining full Rollup API compatibility. Was originally looking at ESM import/exports like a bunch of new tools are doing, but it's not a great solution because it cripples the capability of HMR and results in memory leaks because of the cache busting technique. Using Nollup's existing chunking mechanism will work a lot better since it's just a matter of splitting every file into a chunk.

Going to be doing some prototyping in the spare time, but don't expect anything soon! :)

@charlag
Copy link
Contributor

charlag commented Dec 7, 2020

I did some research and it seems like Webpack also uses eval by default for development and lists it as the fastest tool:
https://webpack.js.org/configuration/devtool/

@charlag
Copy link
Contributor

charlag commented Dec 7, 2020

Also regarding the screenshot above: you can get better stack if you click on the error to expand it 😉

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

No branches or pull requests

3 participants