Skip to content
This repository has been archived by the owner on Mar 30, 2023. It is now read-only.

transform will not mutate imports with '.' in name #9

Closed
ujwal-setlur opened this issue Dec 22, 2019 · 7 comments
Closed

transform will not mutate imports with '.' in name #9

ujwal-setlur opened this issue Dec 22, 2019 · 7 comments

Comments

@ujwal-setlur
Copy link

I am trying to import a module api.service, but this transform does not append the the js extension. It's due to this code:

// only when module specifier has no extension
if (path.extname(node.moduleSpecifier.text) !== '') return false

Any reason for this check?

@MicahZoltu
Copy link
Member

Sorry for the delay on getting back to you on this sooner! The notification email got lost during the holidays.

The reason for that check is because we don't want to add a .js extension if one already exists. For example, if the user has import { Foo } from './module.js' we don't want to end up with import { Foo } from './module.js.js'. Similarly, the user may do something like import 'foo.html' which we also don't want to suffix.

We could do some sketchy things like if (extension.length > 4) but that would fail for longer extension names (which are valid). Unfortunately, there isn't a great solution that I can think of that won't break someone. Honestly the best option is probably to just fork the project and remove that check if your projects never have extensions specified in them.

Another potential option would be some sort of config file, though the infrastructure required to get a config file working properly is pretty high relative to the size of this project so I'm a bit hesitant to go down that route.

@MicahZoltu
Copy link
Member

Haven't heard back from you so I'm going to close this. Feel free to re-open it if you want to continue the discussion.

@ujwal-setlur
Copy link
Author

Yeah, we are good.

@ChristianUlbrich
Copy link

This effectively renders this transformer useless. It is very common, to "type" the name in the filename, i.e. api.service.ts, frontend.component.ts. This could easily be solved by not relying on path.extname, but a whitelist of extensions, that won't be transformed. Reasonable defaults would be ['js', 'jsx', 'ts', 'tsx', 'mts', 'cts']...

@MicahZoltu
Copy link
Member

"Useless" seems like an overly strong claim here, though "useless for projects that use . as part of their naming convention (like in your example) does seem correct.

My recommendation is to just fork the project. You only need to change the name property in package.json and npm build & npm test & npm publish to publish to NPM with the new package name. As far as making the desired code change, you could either remove the line mentioned in this issue or change it to check against a list like you have suggested.

@ChristianUlbrich
Copy link

There should not be any restrictions on the kind of import paths. Imports with a . are perfectly legal import parts. If this library has restrictions on import paths it should clearly communicate them, otherwise this transformer, does not solve a the generic problem "to all import paths a .js is added", but only the problem: "to all import paths that do not contain any . a .js is added".

If the answer to a suggested solution, is that I should just fork the project, that is fine to me. But it renders this transformer useless to me and probably to a lot of other users as well, because "sub-typed" filenames (api.service.ts, frontend.component.ts, something.test.ts, ...) are the real world™ if you are developing frontends.

In the end, I wrote my own transformer, because this transformer also suffers from other problems (import type...). Nevertheless at least I got the hint with typescript.updateImportDeclaration and so it had some value to me!

@MicahZoltu
Copy link
Member

The fundamental problem here is that we are doing a transformation on a file extension, which is not particularly well standardized. There are a number of heuristics that one could pick that would work for some subset of use cases, but there are almost no heuristics (and definitely no simple ones) that would work for all users in all scenarios. For this package, I decided to do the simplest mechanism that I believe addresses the needs of most users (including myself importantly) without introducing too much complexity (and thus bugs).

I think what you are proposing (just add .js to everything) is quite reasonable for many users (anyone who doesn't import HTML/CSS/JSON files) and I fully support a fork of this project (with or without attribution) that does exactly that!

suffers from other problems (import type...)

Can you provide more details on this and/or your fix?

In the end, I wrote my own transformer

Is your transformer published to NPM/GitHub? If so I would be happy to link to it in the Readme and this issue so other users wishing the same feature set can find it quickly/easily.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants