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

Publish ES5 modules to NPM #137

Open
scurker opened this issue Feb 16, 2017 · 48 comments
Open

Publish ES5 modules to NPM #137

scurker opened this issue Feb 16, 2017 · 48 comments

Comments

@scurker
Copy link

scurker commented Feb 16, 2017

It seems a common pattern for modules in npm these days is to publish their code as ES5, while keeping ES6 modules in the original source. The idea being is that it doesn't require any consumers to have any knowledge of how external modules are written, it just works out of the box.

Assuming most modules in npm are already compiled, most webpack configs you see tend to ignore node_modules for faster compilation time:

module: {
  rules: [{
    test: /\.js$/,
    exclude: /node_modules/,
    loader: 'babel-loader'
  }]
}

This can cause issues with bundlers if your code isn't ES5 because if you have this configuration, it chokes on ES6 specific code. You can work around this for autotrack by excluding these specific packages:

exclude: /node_modules\/(?!(autotrack|dom-utils))/

However, that can get pretty long if you have a lot of external packages that aren't precompiled. It's also not as straightforward for someone who is starting out and may not be familiar with the inner workings of webpack.

In either case, either this should be documented for bundle users or have published ES5 modules in npm so that it's not an issue. I'd be happy to create a pull request to help.

@philipwalton
Copy link
Member

Thanks for the suggestion. I actually spent a lot of time thinking about this issue before publishing, and ultimately I chose to publish the source modules rather than the built modules.

Here was my initial thinking (I'm happy to reconsider if it becomes a burden for a lot of people):

  • Publishing the source to npm forces users into the transpiler of my choosing, not their choosing. Autotrack source code is written to work with either babel or closure compiler (which produces much smaller output than babel), but if I published babel versions, closure compiler users wouldn't be able to get those gains.

  • Publishing transpiled source files means those transpiled source files all have to include every helper function (e.g. _createClass(), etc.) with them. This means you can't optimize your build to only include those once.

  • I feel pretty strongly that autotrack code should never affect load performance, so I think it's an antipattern to import it in your main site bundle (unless you're using dynamic import() and code splitting features). That's why I included the build tool with this release, so folks could build it separately and load it via <script async>.

  • excluding /node_modules/ is a performance win on the initial build, but in my experience rebuilds are the main area where performance matters, and that won't be an issue since you're probably not changing the contents of node_modules during development.

I'd consider publishing both the source and built files, but I'd want to make sure there was a real demand for it before doing the work to support it.

Thoughts?

@scurker
Copy link
Author

scurker commented Feb 17, 2017

Thanks for your thoughts, it's great to see some solid thinking behind this.

However, since it does seem this does deviate from the norm for NPM published packages it would be nice if the loading via npm section mentioned you will need to be sure you transpile down to ES5 since it will ultimately be used in the browser.

@matthewlein
Copy link

matthewlein commented Feb 17, 2017

Thanks @scurker !, I just spent 5-6 hours troubleshooting babel, webpack, grunt, the upgrade to webpack 2, downgrading to webpack 1, bouncing around commits, changes. It was because babel was not processing node_modules, as you said.

@philipwalton I think this is misleading:
If you use npm and a module loader that understands ES2015 imports (e.g. Webpack, Rollup, or SystemJS), you can include autotrack in your build by importing it as you would any other npm module
The imports worked fine, yes, but in my case the default function params (and maybe other es6 features) did not without babel processing. You definitely need a blurb in there and the upgrading guide.

exclude: /node_modules/, is the defacto standard everywhere I've seen, so I'd probably put a section on babel and adding exclude: /node_modules\/(?!autotrack)/,

The end fix is very simple, but understanding the problem these days is a quagmire of interconnected tools, not so clear error messages, and mountains of outdated config recommendations...

Edit: also worth noting that I did try removing the exclude on node_modules completely early on, but ran into another module that had some babel settings and a not so clear error message.

@scurker
Copy link
Author

scurker commented Feb 17, 2017

@matthewlein FYI you also need dom-utils in your exclusion list since it's also published as ES6.

I wish there was a better community standard around this, because I think Philip has perfectly valid reasons to not transpile code but there's not an official way to make those intentions clear.

@matthewlein
Copy link

Huh, haven't hit any errors yet without it. I will keep that in mind if I see anything.

@matthewlein
Copy link

Derp. There it is, dom-util required as well.

@mattecapu
Copy link

This is the reason I'm still sticking with v1. Compiling a module is non-standard practice so a lot of the tools I use are not really good at this. For me, it was easier to just not update autotrack.

I also think publishing ES2015 is great, but the ecosystem is not ready yet, mainly because there is still no agreement on how to do this.
I would have waited for the ES Modules proposal to land in Node.js. This will set a standard that any tool will eventually support.

May I propose to publish an ES5 version on the v1 branch? This would give users an opton.

@cycomachead
Copy link
Contributor

I also just spent a couple hours on this. I 👍 the recommendations about improving the docs as well, and had the exact intuition about "ES2015 compatibility". Perhaps we could just link to this issue?

To be honest, adapting our webpack exclusions isn't tricky as long as I know that's what needs to be adapted.

@ggregoire
Copy link

Hi, just to add my two cents (sorry I'm late):

Create-React-App's build process doesn't transform the libs in node_modules.

From Dan (React team & author of CRA):

No, this is not supported because builds will get much slower. We don't recommend using libraries that don't precompile their code to ES5 but are supposed to be usable in ES5.

@bryce-larson
Copy link

My two cents:

After a couple of hours, was able to figure out the root issue and fix my build. Finding this thread helped, but it took a while to understand why this was the issue and further complicated since I am using Typescript. Here is the error I was getting:

ERROR in bundle.js from UglifyJs
Unexpected token operator «=», expected punc «,» [bundle.js:13291,12]

IMO, this is not the default format that npm modules are published. This is a hard / bigger question but until community figures the way to move forward from ES5 it would be nice if this repo followed the current defacto standard.

I opt'ed to add babel to just to get this module to work, here are the steps if anyone else is interested.

npm install babel-loader babel-core babel-preset-env babel-preset-es2015 --save-dev

webpack v2

  module: {
    rules: [
      {
        test: /\.tsx?$/,
        use: { loader: 'ts-loader' },
      },
      {
        test: /\.js?$/,
        use: { loader: 'babel-loader' },
        exclude: /node_modules\/(?!(autotrack|dom-utils))/,
      },
    ]
  },

@philipwalton
Copy link
Member

I've spent a bit of time looking into what it would take to publish the plugin source files as ES5 so they could be more easily consumed via webpack with most users' exclude: /node_modules/ configuration, and I don't think there's a good solution.

The problem is, since autotrack depends on my dom-utils package, transpiling all the autotrack source files via babel into a /dist folder isn't sufficient, I'd also have to release a version of dom-utils with its files transpiled. But the problem there is even if I release both autotrack and dom-utils with transpiled /dist folders, there's no way to dynamically tells the import paths that were requested with a dist in their name to continue to use dist (rather than lib) in their sub-imports.

That means my options are to either (1) publish only babel-transpiled ES5 versions of all the plugins (which I don't want to do since closure compiler-transpiled versions are ~40% smaller), or (2) I can continue to publish ES2015 versions and ensure this issue is as clear as possible in the documentation.

At this point I'd much rather do option (2), since it's ultimately the most flexible and future-proof, and it still works with all existing tools (just not with all existing configurations of those tools).

On other hand, if I do option (1), I'd be preventing anyone who wants to use a transpiler other than babel from doing so, and I'd be forcing all consumers of autotrack to download far more code than they need.

I also want to point out that I already am providing an ES5 version of autotrack that users can include in their builds, it's just not referenced in package.json, so it doesn't get important when using the node module resolution algorithm.

I people want, I'd be willing to update the main key in package.json to point to the ES5 version. That would solve this problem for folks using just import 'autotrack'. Unfortunately, it still wouldn't solve the problem for folks who want to import individual plugins (though maybe that's a less common use-case).

@philipwalton
Copy link
Member

I also want to reiterate that, even if you're using webpack to build your app, you don't have to bundle autotrack with the rest of your code. It's actually much better to create a custom build and load it separately via <script async> (as describe in the installation and usage section).

And if you really want to use webpack to bundle autotrack, then you should definitely be using code splitting and you can still transpile autotrack via babel-loader with the configuration @surker described.

@michaelBenin
Copy link

Another way to solve this problem without needing a special case in the webpack config:

import 'autotrack/autotrack.js';

@philipwalton
Copy link
Member

@michaelBenin doing that will include all plugins, which kinda defeats the purpose of us using a bundler since that file is already bundled/minified/optimized.

You also don't want to include it in your site's main JavaScript file as it'll slow down the load unnecessarily (I say unnecessarily because analytics code never needs to be loaded up front).


Just to reiterate to everyone on this thread, I get the appeal of loading autotrack via webpack, but unless you're using webpack's code splitting features, you're actually making your site slower by including autotrack in your main bundle.

It's much better to load the full version of autotrack via <script async> (as shown in the README) than it is to bundle a subset of the autotrack plugins but include them in your main bundle and (as a result) slow down your site's initial load.

zackhsi added a commit to IrvineUniversityParkLibraryFriends/irvineuniversityparklibraryfriends.github.io that referenced this issue Apr 13, 2017
zackhsi pushed a commit to IrvineUniversityParkLibraryFriends/irvineuniversityparklibraryfriends.github.io that referenced this issue Apr 13, 2017
@bennidhamma
Copy link

Add me to the list of folks who have lost more than one hour to this decision. I appreciate that for some use cases, being able to save, what is it, about 10kb?, might seem important.

But for lots of users, the engineering hours spent trying to optimize that 10kb of savings may not be worth it. Our bundle is about 700kb currently. I'm not going to defend that, but it's also perfectly acceptable for our use case. My priority right now is getting a product out to market that is fast (enough) and gets to our users quickly so we can collect usage data, gather feedback, start generating revenue, etc.

For use cases like ours, we are more than happy to take a 10kb hit on an important library if it lets us get moving fast.

I was blown away by how powerful your library is, kudos to you. And I tossed the changes into a PR, moved on to the next feature in a few hours (bigly savings there again, congratulations - terrific job). But then to come back later and see that our builds were failing because of this sort of non-standard practice was definitely a bummer.

Could you ship prebuilt versions for babel and closure? Again, I have read all of your concerns, and those are all great, but for many use cases, there really isn't anything wrong with developers wanting to keep their dev process streamlined and efficient - that's also a feature.

"Compliance is a feature" how's that for a quote?

@scurker
Copy link
Author

scurker commented Apr 24, 2017

My understanding with regards to Webpack v2 and Rollup is that pkg.module should be able to be used in tandem with pkg.main. See pkg.module. This should allow for your pkg.main to be the closure compiled commonjs bundle, while pkg.moduleis still ES6. Would this not be an option to have both compiled and original sources?

@philipwalton
Copy link
Member

@scurker the problem with using the main/module property of package.json is that, unlike most JavaScript libraries, autotrack doesn't have a single entry point if you're consuming a subset of the full plugin list.

I provide an ES5 version of the library with the full plugin list, and I expose the source code for users who want to include just specific plugins.

As far as I know, package.json doesn't support any config options for this type of library (one with multiple entry points).


@bennidhamma if you don't care about the extra size to your bundle and you want to bundle with webpack instead of using <script async>, you can do so with require('autotrack/autotrack') (which will pull in the ES5 built version).

Though, as I've said a few times in this thread, I strongly recommend against doing this. It's far better to just use <script async> and load the full, built version separately than it is to include it in your main bundle.

Could you ship prebuilt versions for babel and closure?

I already ship an ES5 version. Arguably, the version that gets loaded when you do require('autotrack') should be this ES5 version, though I can't change that without doing a major version bump.

@bennidhamma
Copy link

Ahh, my bad. I misunderstood the documentation. Thanks @philipwalton :)

@coreyward
Copy link

coreyward commented May 10, 2017

I want to jump in here and say this bit me as well—over 10 lost hours over the last two days, and wasted the time of the Heroku support team, the Heroku ruby buildpack dev team, and the Rails webpacker team.

At the end of it all, I still have no idea how to actually utilize analyticsjs-boilerplate with autotrack in production (it's working perfectly in development). Unfortunately, it's already cost me thousands of dollars, so I'll be tearing it out and going back to the default tracking snippet with some smart tweaks and additional calls, loaded however the hell Google wants to load it, because what's important is that I can ship the feature and move on with my work.

Please reconsider whatever has resulted in this experience.

@zackhsi
Copy link

zackhsi commented May 10, 2017

@coreyward (and whoever else) my understanding of what @philipwalton is saying is that he doesn't want to sacrifice performance for ease of use and possible incorrectness (I don't claim to understand all the details mentioned in this issue). I think that is fine. Perhaps what we need is more understandable documentation on how to do things the Right Way.

For me, this commit fixed autotrack. Basically, with this command I generated autotrack.custom.js. I put that in my repository's assets, and then load it asynchronously.

Hope that helps.

@coreyward
Copy link

@zackhsi Unfortunately, the solution you're using doesn't work for me since I'm using another library by @philipwalton that, somewhat confusingly, doesn't follow his advice, and imports from autotrack. It's also not available compiled to ES5.

If I were using autotrack by itself, I could copy-paste pre-built JS files into my app, modify them as needed, and load it all asynchronously with script tags. That's what I've done in the past, without bothering with yarn/npm, webpack, babel, or a dozen other dependencies.

@philipwalton
Copy link
Member

@coreyward sorry you lost time to this. If you can point to anything thats missing from or misleading in the documentation, I'm happy to fix it or clarify.

The truth is webpack is complex, and lots of people run into issues using it. That's why I ship both (1) a pre-built, pre-minified, ES5 version of the library in the repo that people can load as-is via <script async>, and (2) a build tool to generate your own custom version (again, minified and transpiled to ES5).

All of the issues people have raised in this thread pertain specifically to how to load autotrack with webpack and babel-loader. You can completely avoid that complexity by not loading autotrack via webpack and babel-loader.

As for the analyticsjs-boilerplate repo, one of the reasons I created it was to give folks an example of how to properly load autotrack with webpack. And all I mean by "properly" is just not including it in their main site bundle. The code I use in that repo uses dynamic import(), so Webpack knows how to split that up and load it lazily.

@philipwalton
Copy link
Member

@zackhsi I think this comment is very much on point:

Perhaps what we need is more understandable documentation on how to do things the Right Way.

Feel free to reach out to me any time to offer feedback on this library or any best practices we (Google) promote. If anything is confusing or needs clarification, we want to know.

@brentmclark
Copy link
Contributor

brentmclark commented Jun 2, 2017

@philipwalton thanks a ton for this context; it certainly spared me many of the pitfalls that others have encountered. FWIW, it still took me a few hours of failing to get autotrack working via webpack before I took the time to read this thread. I am not complaining by any means; simply sharing my experience in the hopes that others can get to the end of this rainbow even faster than I did.

Can I recommend some sort of very noticeable disclaimer immediately inside of the Loading autotrack via npm section that reads something akin to the text below?

The following approach is not recommended unless you have a really good reason to use it (e.g. code splitting). The recommended approach is via <script async> as shown above

@philipwalton
Copy link
Member

@chaffeqa

At this point anything that is up to date is expected to be falling into the webpack standard. This means you can simply add the module, and import it as if its esX (and rely on your own .babelrc to dictate that).

I disagree strongly with this. If you live in the react/babel/webpack world, then perhaps it seems like that, but the web development ecosystem is quite a bit larger than that ecosystem, and I'd rather offer flexibility that benefits everyone (and most importantly, minimizes the potential hurt to end users).

Currently adding autotrack as a node_module forces you to add: chalk, dom-utils, fs-extra, glob, google-closure-compiler-js, gzip-size, rollup, rollup-plugin-memory, rollup-plugin-node-resolve, source-map
And locked versions, which means possibly multiple in your project.

You only have to add autotrack and dom-utils.

And to be clear, this isn't an autotrack issue, it's a Webpack and babel-loader issue. Webpack needs to make it easier to specify which modules (and their dependencies) should or shouldn't be included by a particular loader.

And if you don't want to worry about that, then you can just remove the line exclude: /node_modules/ from your config, and everything will work fine. Note that this is the default setting for babel-loader, it's all the tutorials and sample code on the web that have made excluding node modules seem like the default.

Also, this is an area where babel-loader could be drastically improved. It should be creating a disk cache of all transpiled files and only re-transpiling the ones that have changed since the previous transpile.

Embraces Build Systems
The biggest win the community is moving towards is to try and enforce best practice by enforcing adhering to build systems.

I'm actually embracing build systems the most by publishing ES2015 source files, as they're far more compatible with a wide range of build systems. By publishing the source files as transpiled ES5 I'm not embracing build systems at all, I'm forcing my build onto others.

All that could be prevented if package.json had either "jsnext:main": "lib" or "browser": "lib" defined pointing to the un-transpiled source (I believe)

This isn't quite true. jsnext:main/module are for versions of your code with import/export syntax rather than require/module.exports. It still doesn't solve the problem of all the other ES2015 syntax features that would need to be transpiled if you want to supported non-ES2015 browsers (which is only IE11 at this point).

@philipwalton
Copy link
Member

Since confusion keeps coming up around this, I think it makes the most sense to update the main entry in package.json to the ES5 version of the code.

Here's what this will do:

What will change:

For users who import the entire autotrack export, e.g.:

import 'autotrack';

They will get the ES5 version in the main repo, so they won't see any errors when loading via webpack and transpiling via babel-loader using the most common configuration options (i.e. excluding node_modules).

This is effectively the same as what you can already do by importing autotrack/autotrack.js today.

What will not change:

Users who only import select plugins, e.g.:

import 'autotrack/lib/plugins/event-tracker';
import 'autotrack/lib/plugins/outbound-link-tracker';
import 'autotrack/lib/plugins/url-change-tracker';

They will still need to transiple the source from ES2015. As I've explained above, I cannot publish all individual plugins as ES5 because they have dependencies on modules that are published as ES2015, and import statements cannot dynamically update their path based on the path of the requester.

Does this seem reasonable to everyone?

@xsynaptic
Copy link

I'm late to the party but that seems reasonable. I appreciate the availability of ES6 source files and have no problem working with this package as I whitelist paths for transpiling (autotrack and dom-utils in this case) with webpack rather than exclude node_modules. Still, at the present moment doing all this fancy optimization stuff is more in the power users domain, and sticking to the principle of least surprise by offering a compiled ES5 entry point sounds like the right thing to do.

@philipwalton
Copy link
Member

It's also worth pointing out that making import 'autotrack' import the bundled, ES5 version will prevent a build system from de-duping shared dependencies.

This is probably not a major concern for most autotrack users, but it's another argument on the side of it making the most sense to publish source files rather than bundled and compiled files.

Publishing the source files will always give tools the most flexibility and power to make optimizations.

@scurker
Copy link
Author

scurker commented Jul 5, 2017

That's probable a good compromise that helps reduce some confusion. Is there any chance the documentation can get updated as well for importing individual plugins?

@justin-hackin
Copy link

justin-hackin commented Aug 23, 2017

I am using https://github.com/vuejs-templates/webpack as boilerplate and I found the proposed exclude solutions didn't seem to help. Finally decided to forget about it and just do import 'autotrack/autotrack.js' but would appreciate any insights as to why exclude: /node_modules\/(?!(autotrack|dom-utils))/ didn't work. I tried first just adding to babel-loader but then I also tried adding it to eslint loader but neither helped.

@philipwalton
Copy link
Member

philipwalton commented Aug 24, 2017

@justin-hackin here's a working webpack example from my analyticsjs-boilerplate project. Perhaps compare it to that and see what's different.

dguo added a commit to dguo/dailylore that referenced this issue Aug 25, 2017
In autotrack v2, the published package is in the form of ES6 modules,
and this is causing an issue with Webpack minification.

See googleanalytics/autotrack#137

I tried the proposed solution of excluding it from Babel, but it
didn't work on the first try, and I don't want to spend any more
time on it. We only use one autotrack plugin anyway.
dguo added a commit to dguo/dailylore that referenced this issue Aug 25, 2017
In autotrack v2, the published package is in the form of ES6 modules,
and this is causing an issue with Webpack minification.

See googleanalytics/autotrack#137

I tried the proposed solution of excluding it from Babel, but it
didn't work on the first try, and I don't want to spend any more
time on it. We only use one autotrack plugin anyway.
@koresar
Copy link

koresar commented Dec 22, 2017

Like @justin-hackin I also was trying to apply autotrack to https://github.com/vuejs-templates/webpack -based application. None on the above fixes helped.

I ended up copying the entire ./lib directory of this folder, as well as dom-utils source code.
Added one more line to .eslintignore. Changed the import paths to local file system and off we go - treeshaking and stuff works.

@lucasmike
Copy link

lucasmike commented Jan 4, 2018

I got another solution which is somewhat in between loading external scripts and a full webpack compile:

Step 1: copy analytics.js and autotrack.js to your webpack static folder.
Step 2: in your html entry file, add:

<script async src="static/analytics.js"></script>
<script async src="static/autotrack.js"></script>

you can change the names of the files as you wish if you don't like the defaults

Step 3: back in your own javascript code, initiate the global ga variable:

// eslint-disable-next-line
window.ga=window.ga||function(){(ga.q=ga.q||[]).push(arguments)};ga.l=+new Date

window.ga('create', 'UA-XXXX-1', 'auto')

// Replace the following lines with the plugins you want to use.
window.ga('require', 'eventTracker')
window.ga('require', 'outboundLinkTracker')
window.ga('require', 'urlChangeTracker')
// ... more plugins
window.ga('send', 'pageview')

works for me.
No problems with bundling -> static files
No problem with performance -> async load

@gaearon
Copy link

gaearon commented Jan 13, 2018

FYI, we're starting the work to compile deps with babel-preset-env in Create React App: facebook/create-react-app#3776

Let us know if you have feedback about how this should work.

@garygreen
Copy link

garygreen commented Feb 22, 2018

I came across this same problem today - deployed our website with webpack simple autotrack entry thinking everything was all good but of course BOOM doesn't work in IE11 and who's to blame? autotrack because it didn't load a precompiled dist version. Caught with my pants down, unbelievable. 😄

Seems package.json main still points to the uncompiled version - I can compeltely understand @philipwalton view point that providing the transpiled code and then bundling it again with another bundler adds unnescarry overhead. However, I agree with the sentiments made above by other people that the general consensus in the community is to always set "main" as the transpiled + bundled version, that way you don't run into problems with whatever customisations that partiuclar plugin has done when you come to bundle stuff up.

Also @philipwalton I notice you said:

You only have to add autotrack and dom-utils.

But that's not true, according to the package.json you need these as well (they are not in dev devepencnies which I find odd):

"chalk": "^1.1.3",
"fs-extra": "^3.0.1",
"glob": "^7.1.1",
"google-closure-compiler-js": "^20170423.0.0",
"gzip-size": "^3.0.0",
"rollup": "^0.41.4",
"rollup-plugin-memory": "^2.0.0",
"rollup-plugin-node-resolve": "^3.0.0",
"source-map": "^0.5.6"

Surely those are dependencies when your compiling a custom version using your own autotrack command right?

I'm not sure what the solution is here but I think at minimum main should point to the fully compiled ES5 module, and if anyone wants to build their own version they should be able to do so easily with whatever bundler they use (maybe instructions would suffice)

@garygreen
Copy link

garygreen commented Feb 22, 2018

I think @philipwalton your main concern is with this, right?

import 'autotrack/lib/plugins/event-tracker';
import 'autotrack/lib/plugins/outbound-link-tracker';
import 'autotrack/lib/plugins/url-change-tracker';

Couldn't that import as uncompiled and then is it possible to have the compiled version as well, but just under a different directory?

import 'autotrack/dist/es5/lib/plugins/event-tracker';
import 'autotrack/dist/es5/lib/plugins/outbound-link-tracker';
import 'autotrack/dist/es5/lib/plugins/url-change-tracker';

I'm a bit of a babel noob so not sure if that is possible, but wouldn't that allow for all possibilities? That way people can consume and bundle whatever version they like.

For me the worst case scenario is to use a custom autotrack command to bundle dependencies up because it seems very "custom" and just unnescary complexity for developers to have to resort to that to just tree shake stuff away they don't need... just imagine if every npm package had it's own command to transpile + bundle stuff in it's own special custom way.... that would be my idea of a nightmare.

@philipwalton
Copy link
Member

@garygreen

deployed our website with webpack simple autotrack entry thinking everything was all good but of course BOOM doesn't work in IE11 and who's to blame? autotrack because it didn't load a precompiled dist version.

I think your lack of cross-browser test coverage is to blame :)

Joking aside, I haven't made the changes I mentioned above in autotrack because I'm starting to see more and more websites deploying ES2015+ versions of their code (I actually have an article explaining how to do this with an ES5 fallback).

And given that create-react-app (one of the main blockers here) is now transpiling all dependencies by default, I think the community is going to start moving in that direction pretty quickly.

But that's not true, according to the package.json you need these as well (they are not in dev devepencnies which I find odd) ... surely those are dependencies when your compiling a custom version using your own autotrack command right?

They aren't in dev dependencies because they're needed for the custom compiler binary.

Couldn't that import as uncompiled and then is it possible to have the compiled version as well, but just under a different directory?

It's possible, but it'd only work if it and all of it's dependencies used the exact same dist/es5/* style naming and conventions. I think that's a lot to ask of module authors, especially when it's so easy (in fact, it's the default) to not exclude node_modules from transpilation.

@garygreen
Copy link

garygreen commented Feb 22, 2018

I'm starting to see more and more websites deploying ES2015+ versions of their code (I actually have an article explaining how to do this with an ES5 fallback).

Interesting article, it definitely makes a lot of sense as the majority of browsers support ES2015 natively. I think just improvements the docs would help

They aren't in dev dependencies because they're needed for the custom compiler binary.

Kind of annoying though that you need to install those dependencies even if you won't be using your custom build CLI tool to build a custom version, or want to bundle with webpack.

I think that's a lot to ask of module authors, especially when it's so easy (in fact, it's the default) to not exclude node_modules from transpilation.

Well the webpack babel-loader advises in it's docs to exclude node_modules, reason being that most npm packages can be consumed with the dist/transpiled version. It's not really much of a problem though cos you can override it with alias option in webpack, if you really want to use the dist version - I'm guessing that you can do that with autotrack too?

image

I totally understand you wanting to push towards a better way of bundling, it does add complexity to using the library though.

For example, you require dynamic-import-system-import as an additional plugin, this isn't standard and it means you need to create a special exception in your webpack config just to transpile autotrack.

What about other package developers who use special plugins / options as well? I can imagine your bundler config getting pretty big supporting different ways of transpiling. Typescript? I guess that's a separate issue though.

@philipwalton
Copy link
Member

Well the webpack babel-loader advises in it's docs to exclude node_modules...

babel-loader shouldn't be, and we're working with them and others to remove this line from their code samples and tutorials.

...reason being that most npm packages can be consumed with the dist/transpiled version.

The reason they advocate it is because it's faster. But as it's becoming more and more common for authors to publish ES2015+ code, it's not good for these sites to be including that line in their basic usage guidelines.

It's also not true that all packages can produce a dist/transpiled version. If you have any third-party dependencies then bundling them into a dist version will lead to code duplication. Bundlers already have a mechanism for dependency de-duping, but if module authors are bundling their dependencies that work is wasted.

Again, note that all of this works perfectly, out of the box, with the default configuration of webpack and babel. It's only problematic if you exclude node_module dependencies from transpilation when those dependencies need it.

For example, you require dynamic-import-system-import as an additional plugin, this isn't standard and it means you need to create a special exception in your webpack config just to transpile autotrack.

That's not true, autotrack doesn't use dynamic import() anywhere in its code. I'm not sure where you're seeing that.

@scurker
Copy link
Author

scurker commented Feb 27, 2018

I haven't had time to play around with it myself, but with the release of webpack 4, build tools are moving towards faster builds where ignoring node_modules may no longer be as beneficial. I wouldn't necessarily agree that everyone is publishing exclusive ES2015+ code, but things are moving in that direction where that is more of a reality.

@gaearon
Copy link

gaearon commented Oct 2, 2018

Create React App can handle standard modern JS in dependencies starting with 2.0.
https://reactjs.org/blog/2018/10/01/create-react-app-v2.html

apepper added a commit to Scrivito/scrivito_example_app_js that referenced this issue Jan 7, 2019
See googleanalytics/autotrack#137 for details, why autotrack also needs dom-utils.
woeltjen added a commit to thebeetoken/beenest-web that referenced this issue Feb 26, 2019
...to avoid SyntaxError in legacy browsers

googleanalytics/autotrack#137
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