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

Don't generate sourcemaps #1023

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Conversation

realityking
Copy link
Contributor

Based on the discussion in eslint/eslint#14098 I took a look at all the files in eslint's dependencies and I noticed that the SourceMaps in acorn make up ~46% of its package size. While only a very small part of the total, they are the two largest single files in all of eslint.

There's obviously a trade off between a smaller package and the potential better debugging enabled by SourceMaps. Since Node.js doesn't load SourceMaps by default nor does Webpack without a plugin, I think they're of very limited value. The only exception would be someone loading the UMD directly in a browser - I'm not certain how common this is for acorn.

I think it's worth considering to no longer generate & ship these files to reduce the package size by ~46%.

@RReverser
Copy link
Member

@realityking That issue seems to be mostly about number of files (since lots of small files are fairly expensive on many filesystems), rather than their sizes? In that case I doubt size of a single source map file matters that much, and the downside is that it would break debugging for users who do rely on it.

OTOH I wonder if at this point Acorn could just rely on ES modules and not transpile altogether, leaving it to the end users. The ecosystem is certainly moving in that direction the last few years. @marijnh what do you think?

@marijnh
Copy link
Member

marijnh commented Apr 9, 2021

OTOH I wonder if at this point Acorn could just rely on ES modules and not transpile altogether, leaving it to the end users.

I tried this with CodeMirror 6 (a new package) and immediately got a bunch of complaints (node ergonomics around ES modules are still not great—can't use them from the repl for example). So since we already have this build system in place, and changing it would be a very breaking change, I think it's best to leave things as they are for the foreseeable future.

I've stopped shipping source maps in some other packages (since indeed, they are big). I wouldn't be opposed to doing the same here.

@realityking
Copy link
Contributor Author

@RReverser It's not directly about the number of files, it's about disk churn as Chromium vendors their npm dependencies. SourceMaps don't diff very well so any change will cause the whole file to be rewritten.

@marijnh I agree, it's too early for ESM-only packages. Since they can only be loaded asynchronously from CommonJS files they cause major downstream changes and the ecosystem is far from ready. Dual mode packages - like acorn are good solution.

What might be considered for the next major version is switching from UMD to CommonJS for the not-ESM build. They're smaller and a little bit faster. AMD is dead and I don't think acorn is the kinda library people load into the browser directly.

@marijnh
Copy link
Member

marijnh commented Apr 10, 2021

and I don't think acorn is the kinda library people load into the browser directly.

You'd be surprised. But yeah, on the next major version I'd be okay with switching to CJS (no concrete plans for a major version bump in the near future though).

@RReverser You okay with merging this?

@RReverser
Copy link
Member

@RReverser You okay with merging this?

I don't mind I guess.

@marijnh marijnh merged commit e6f4c2d into acornjs:master Apr 12, 2021
@marijnh
Copy link
Member

marijnh commented Apr 12, 2021

I've tagged a version 8.1.1 without the source maps.

@realityking realityking deleted the no-sourcemaps branch April 12, 2021 07:43
@realityking
Copy link
Contributor Author

Thank you @marijnh!

@TimvdLippe
Copy link
Contributor

I just discovered this PR and wanted to say: thank you! Yes while the ESLint issue was concerning number of files, we also care about file size. Not shipping sourcemaps in NPM packages will help a bunch 😄

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.

None yet

4 participants