Neither default entry or browser is ready for webpack inclusion

[dcousineau]

I just ran across a bug that isn't a high priority but I wanted to report it and ask what way you would prefer to proceed before opening a PR:

In my project I do import focusWithin from 'focus-within'; /* ... */ focusWithin(document); in my entry-point file. Doing so results in my vendors bundle including focus-within/index.js. I do not include the browser file manually in its own <script /> tag for two main reasons: existing build chain and performance.

A user with Safari 9 (this is why the bug isn't a high priority) visited my application an immediately errored out with: SyntaxError: Unexpected keyword 'const'. Const declarations are not supported in strict mode.

While my application handles the babel transformations required for our supported browsers list (which admittedly does not include Safari 9 but does convert all const/let to var still), as is standard convention we do not transform any node_modules as we expect them to be pre-compiled. We have resolve the issue by carving out an exception and passing focus-within through our babel transformation.

Looking at focus-within I think there are three possibilities for resolution going forward:

1. Update the build config to transform the const/let into var in the index output(s), thereby producing a more strict ES5 compat build.
2. Update the build config to leave a module.exports ... line so it becomes eligible for inclusion in common Webpack flows
3. Ignore this case entirely and provide instructions in the documentation on how to force babel processing for users like me.

I am unfamiliar with rollup (I poked around to see if I could open a PR quickly) but am more than happy to do the leg work on whichever of the above solutions you would prefer for your project.

[jonathantneal]

This is an interesting request, but I don’t see a compelling reason to precompile the Node version for browsers when that is already being done using the browser.js file. Otherwise, if you wish to compile it for a browser, you’ll need to transform it from Node 6 to your browsers list.

as is standard convention we do not transform any node_modules as we expect them to be pre-compiled

I’m not sure that’s a standard convention, but I do understand what you are saying.

The node version of this library supports Node 6 environments with specific distributions for CommonJS and ModuleJS (see https://unpkg.com/focus-within@3.0.2/). This follows Node.js long term support.

I’m surprised you are not already compiling packages from node_modules whenever you replace import or require statements, as those packages commonly reference other packages themselves.

The browser version of this library is pre-compiled for a large set of browsers and would handle this for you. (see https://unpkg.com/focus-within@3.0.2/browser.js or require('focus-within/browser')).

For maintenance reasons, I distribute nearly all of my packages the same. It narrows down bespoke problems if one package is distributed differently than others. I’d like to continue following Node LTS for the packaged version and a conservative browsers list for the browser version. Could you provide some more insight into why I should standardize my distributions for Node 6 and strict ES5?

The environment targets for rollup are configured here: https://github.com/jonathantneal/focus-within/blob/master/.rollup.js#L7

[dcousineau]

I’m surprised you are not already compiling packages from node_modules whenever you replace import or require statements, as those packages commonly reference other packages themselves.

Webpack handles import/require resolution (and shimming/wrapping of the module system to run in a browser) which means the overhead of running Babel is not required to handle transpiling node_modules (the performance hit is quite large given the sheer number of node_modules files that get run).

Because the browser.js file strips the module.exports (https://github.com/jonathantneal/focus-within/blob/master/.rollup.js#L59) webpack can't correctly include and provide the function correctly (import focusWithin from 'focus-within/browser'; /* ... */ focusWithin(); would fail because nothing was exported so the function could never be called. In fact the file wouldn't be included at all because Webpack tree-shaking would notice the function provided was never referenced, because it wasn't exported, and therefore can be stripped from the final build).

A good solution would be not stripping the module.exports so module systems like Webpack can include the browser file.

I’m not sure that’s a standard convention, but I do understand what you are saying.

The official docs in Webpack for utilizing Babel recommend excluding node_modules: https://webpack.js.org/loaders/babel-loader/#usage

create-react-app by default sets up webpack config to whitelist app source only (thereby excluding 3rd party or vendor libs in node_modules).

---

One thing I'm curious about, why the support for node lts? What is the use case for running focus-within (specifically the browser polyfill, not the postcss plugin) within the node runtime environment?

[tillhoerner]

I'm having the same issues as @dcousineau on this. Maybe a solution would be to compile the browser.js to a UMD Module instead of compiling it to CommonJs and than removing the module.exports. This is mentioned on the Rollup Docs in the Compatibility section.

A UMD Module would work when included as script tag and also when using require('focus-within/browser') with webpack.