search

denoland / deno_ast

Source text parsing, lexing, and AST related functionality for Deno
https://crates.io/crates/deno_ast
MIT License
155 stars 46 forks source link

fix: MediaType from content type with conflicting extension #33

Closed kitsonk closed 3 years ago

kitsonk commented 3 years ago

@dsherret I thought about the issue you pointed out, and I think I had it wrong, in that if we have a content type of application/javascript we should respect that, assuming the remote did the transpile already and is clearly overriding automatic content types, except for situations where it would be a type only file anyways (.d.ts, .d.mts, .d.cts) where clearly something is "wrong" since those don't emit.

I don't know any specific behaviours in the wild, but best to keep the behaviour and document it. Based on that, I also aligned it so that a application/javascript with a .mts file becomes MediaType::Mjs and application/javascript with .cts file becomes MediaType::Cjs.

@bartlomieju @ry while I still have my reservations about it, this whole thing we are having to do for TypeScript 4.5 should also allow us to identify and handle CJS files through the module "pipeline". I guess if we are going to do it, I would rather do it properly. 😒

bartlomieju commented 3 years ago

@bartlomieju @ry while I still have my reservations about it, this whole thing we are having to do for TypeScript 4.5 should also allow us to identify and handle CJS files through the module "pipeline". I guess if we are going to do it, I would rather do it properly. 😒

What are your suggestions here? Are you saying that loading of CJS modules should be done via module graph instead of using std/node's functionality? Let me know and I'll work on that to make sure it works nicely.

kitsonk commented 3 years ago

What are your suggestions here? Are you saying that loading of CJS modules should be done via module graph instead of using std/node's functionality? Let me know and I'll work on that to make sure it works nicely.

You are working on moving the CJS loading into Rust, no? You need something that will do your dependency analysis, right? If you ever plan to mix "normal" Deno code and compat code, support for TypeScript, etc. you will also need dependency analysis. The module graph doesn't do loading in CLI, it identifies what will be loaded and is then used to prepare those modules to be loaded.

bartlomieju commented 3 years ago

What are your suggestions here? Are you saying that loading of CJS modules should be done via module graph instead of using std/node's functionality? Let me know and I'll work on that to make sure it works nicely.

You are working on moving the CJS loading into Rust, no?

Not at the moment, I'm still not sure if that's feasible. I will look into that in the second week of November.

You need something that will do your dependency analysis, right?

I guess the dependency analysis can be done independently of actual loading, which could still be done by std/node.

If you ever plan to mix "normal" Deno code and compat code, support for TypeScript, etc. you will also need dependency analysis. The module graph doesn't do loading in CLI, it identifies what will be loaded and is then used to prepare those modules to be loaded.

Agreed. dep_graph in SWC can recognize require calls, but we just ignore them at the moment in Deno.