getify / import-remap

Rewrite ES module import specifiers using an import-map.
MIT License
25 stars 3 forks source link

In `dist/esm/index.mjs`: swallowed error “TypeError: traverse is not a function” #1

Open wolfgang42 opened 1 year ago

wolfgang42 commented 1 year ago

I’m using import-remap in a "type": "module" package, imported as:

import remap from 'import-remap'

However, remap() was mysteriously not doing anything. After some confused debugging, I discovered a try{...} catch(e){} block swallowing all exceptions; when I removed it, I got:

TypeError: traverse is not a function
    at remap (file:///.../node_modules/import-remap/dist/esm/index.mjs:5:917)

Changing traverse(i,o) in that file to traverse.default(i,o) makes this error go away, which makes me think that something has gone wrong in the build process; but I'm not familiar with “moduloze” so I figured I’d report this and see what you had to say.

getify commented 1 year ago

Hmmmm. I'm super confused.

Moduloze takes the original statement var { default: traverse, } = require("@babel/traverse"); in src/index.js file and transpiles that to import traverse from"@babel/traverse";. I believed that to be correct, because the babel docs indicate that's how you import the traverse(..) function correctly.

But in fact, if I try to import it in a test file, it's coming in as an object and not a function.

cmd

It seems clear that Babel's docs are wrong, or at least that they don't work in Node (I tried v14, v16, and v18).

I can't exactly recall, but I don't know if I ever actually tested the ESM build of import-remap (as a library), honestly. I know for sure that the projects where I personally use import-remap, I always use the CLI tool (not the library), and that tool definitely uses the Node/CJS version.

Seems like I definitely would have tested the ESM, but I cann't find any evidence that I did (or that it would have ever worked). So perhaps it's been some long-standing oversight. Ick.

I think we need to figure out definitively from Babel how it's supposed to be imported -- because the docs are clearly wrong -- and then I'll need to see if there's some way to make sure the Moduloze transpilation does that correctly for both ESM and CJS. Since the CJS already works, I need to make sure it doesn't break with accommodating the ESM. Could be tricky, not sure. Might have to change Moduloze, depending on how it shakes out.

wolfgang42 commented 1 year ago

https://github.com/babel/babel/issues/13855#issuecomment-945123514 seems to have the answer, or at least part of it:

The docs are imprecise. They have been written before that Node.js released native support for ES modules, and they assume that you are transpiling your @babel/traverse usage.

If you are using native ESM, whenever the docs use a default import (such as import traverse from "@babel/traverse") you need to get the default property of the default import:

import _traverse from "@babel/traverse";
const traverse = _traverse.default;

This is a side-effect of how the CJS-ESM interop has been designed, and we will fix it in Babel 8 (since it's a breaking change).

...so I guess their transpiling is conflicting with your transpiling, which is annoying. I guess Moduloze needs to special-case this somehow? (I haven’t tried it, but I’d guess import {default as traverse} from "@babel/traverse" would also work.)

Incidentally, I’m not using import-remap after all (it turned out not to be a good fit for my use case, so I ended up making my own little tool to import rewrites using acorn), so this bug is not currently blocking anything on my end.

getify commented 1 year ago

it turned out not to be a good fit for my use case

other than the bug, I would be curious if you could share what was not a good fit.. always helps to understand other use cases besides the ones I have. :)

getify commented 1 year ago

I’d guess import {default as traverse} from "@babel/traverse" would also work

Unfortunately, not... These two are not equivalent:

import {default as traverse} from "@babel/traverse"

import _traverse from "@babel/traverse";
const traverse = _traverse.default;

Very annoying conflict in transpilation strategy. I can't really think of any way for Moduloze to tell the difference... again, since it works fine for the CJS case already.

wolfgang42 commented 1 year ago

I would be curious if you could share what was not a good fit

Sure! I’m building a tool that takes a folder full of assets, and sticks hashes in the filenames (so you can serve them with Cache-Control: immutable). Obviously this also requires rewriting the imports, so that they still point to the right file after the rename. So my plan was to hash everything to the new filenames, then build an import map and remap everything (since this of course changes the contents of the file, rehash and repeat until it stabilizes, or build a DAG but I didn't bother to do that).

So the main reason remap wasn't a great fit is it only does exact-match rewriting, whereas I mainly want to to relative path rewrites. I considered adding support for that to import-remap, but I also want to move some files around in the hierarchy at the same time (so in order to get the right path in the end, the rewriter needs to keep track of both where the the files think they are and also where they’ll actually end up), which is straightforward in the design I ended up implementing but not really something that import maps are prepared to deal with.

As a more minor aesthetic concern, since import-remap round-trips through the Babel AST, it reformats the output file into a different style. (Notably it does entirely its own indentation, but it also does things like moving comments around a bit.) Since I was writing my own logic anyway, I solved this in my implementation by carefully clipping out bits from the original string rather than running codegen on the modified AST.

getify commented 1 year ago

sticks hashes in the filenames (so you can serve them with Cache-Control: immutable)

Just for clarification, you mean something like "foo.js" becoming "foo.28djrkx4h9.js", right?

If so, is the hash the same across the whole set of files, or is it unique to each file?

I always thought just putting all the original named files in a generated sub-directory name, like "28djrkx4h9/foo.js" would be a better approach to this. And that wouldn't need an import-remap if the import URLs were all relative. But I never looked much further into it.

it only does exact-match rewriting, whereas I mainly want to to relative path rewrites

Yeah. The import-map "spec" is now stable (was changing a lot for awhile before it shipped in browsers). I planned to circle back and re-implement all the relative and wildcard matching features once that settled down.

Ironically, now that import-map is close to shipping in all major browsers, this tool will become rapidly less important and eventually mostly redundant.

I also want to move some files around in the hierarchy at the same time

Ahh, well... yeah that's definitely not a feature import-map would likely be helpful with.

the Babel AST, it reformats the output file into a different style.

Sigh. Yes, I've been frustrated with the lossy AST format JS tools use for more than a decade. I tried for years to get a better lossless "concrete sytax AST" format defined and adopted. But the Babel folks (at the time) resisted/refused, so as far as I was concerned, the effort died. And here we are. Most of the working group didn't think it was that big of a deal.

Of course, if TypeScript-in-comments had been a common thing back then, they probably would have cared more. Shrugs.

In any case, aren't you minifying production build assets that you're serving with cache:immutable headers? I would expect that for most usages of import-map.


Thanks for your insight. Very helpful to consider future features (if any). :)

wolfgang42 commented 1 year ago

sticks hashes in the filenames (so you can serve them with Cache-Control: immutable)

Just for clarification, you mean something like "foo.js" becoming "foo.28djrkx4h9.js", right?

If so, is the hash the same across the whole set of files, or is it unique to each file?

Unique to each file. Putting the hash in the directory name would invalidate the entire cache whenever a single file changed.

I also want to move some files around in the hierarchy at the same time

Ahh, well... yeah that's definitely not a feature import-map would likely be helpful with.

I may be going a bit overboard on prettifying my URLs here, it's definitely not a critical feature to have :smiley:

In any case, aren't you minifying production build assets that you're serving with cache:immutable headers? I would expect that for most usages of import-map.

I remember the days when you’d see a cool feature on a website and use View Source to see how it was done :slightly_smiling_face: so since my files are small enough that minifying wouldn’t have any actual benefits, I decided to expose how the sausage worked, so to speak.