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

Error loading in Deno #301

Open
justinmchase opened this issue Aug 12, 2021 · 16 comments
Open

Error loading in Deno #301

justinmchase opened this issue Aug 12, 2021 · 16 comments
Labels
bug Something isn't working

Comments

@justinmchase
Copy link

Describe the bug

error: Import 'https://deno.land/x/[email protected]/src/nodes/Scalar.js' failed: 404 Not Found
    at https://deno.land/x/[email protected]/src/index.ts:22:0

To Reproduce
Steps to reproduce the behaviour.

  1. Install Deno
  2. Run deno eval "export * as YAML from 'https://deno.land/x/[email protected]/src/index.ts'"

Expected behaviour
I expect the module to be loaded successfully, no errors

Versions (please complete the following information):

  • Environment:
deno --version
deno 1.13.0 (release, x86_64-apple-darwin)
v8 9.3.345.11
typescript 4.3.5
  • yaml: v2.0.0-7

Additional context
Full stack:

deno eval "export * as YAML from 'https://deno.land/x/[email protected]/src/index.ts'"
Download https://deno.land/x/[email protected]/src/nodes/YAMLSeq.js
Download https://deno.land/x/[email protected]/src/doc/Document.js
Download https://deno.land/x/[email protected]/src/parse/lexer.js
Download https://deno.land/x/[email protected]/src/schema/tags
Download https://deno.land/x/[email protected]/src/parse/parser.js
Download https://deno.land/x/[email protected]/src/nodes/Alias.js
Download https://deno.land/x/[email protected]/src/nodes/Node.js
Download https://deno.land/x/[email protected]/src/nodes/Scalar.js
Download https://deno.land/x/[email protected]/src/errors.js
Download https://deno.land/x/[email protected]/src/nodes/Pair.js
Download https://deno.land/x/[email protected]/src/parse/cst.js
Download https://deno.land/x/[email protected]/src/visit.js
Download https://deno.land/x/[email protected]/src/parse/line-counter.js
Download https://deno.land/x/[email protected]/src/nodes/YAMLMap.js
Download https://deno.land/x/[email protected]/src/public-api.js
Download https://deno.land/x/[email protected]/src/schema/types
Download https://deno.land/x/[email protected]/src/options.js
Download https://deno.land/x/[email protected]/src/compose/composer.js
Download https://deno.land/x/[email protected]/src/schema/Schema.js
error: Import 'https://deno.land/x/[email protected]/src/nodes/YAMLSeq.js' failed: 404 Not Found
    at https://deno.land/x/[email protected]/src/index.ts:24:0

The files of course are all typescript in the repository and it fails trying to download them because in the code they're all referencing each other as .js. I believe in node it works because its running all the transpiled files after typescript gets them but in Deno that transformation is transparent and it requires the references to be correct.

I know there are a couple of ways to solve this, such as using some auto-esm transpiler, or perhaps you have the JS version published somewhere that I'm not seeing... but I didn't see any documentation in your readme on the way you're intending it to be supported so I thought I'd ask here.

@justinmchase justinmchase added the bug Something isn't working label Aug 12, 2021
@eemeli
Copy link
Owner

eemeli commented Aug 25, 2021

It seems s bit surprising that this would fail in Deno, given the TypeScript itself is fine with resolving .js into .ts. I don't use Deno myself, so if there's a simple fix for this I'd be interested. The sources should be pretty Deno-compatible, and I'd really rather avoid adding a separate build for it.

@justinmchase
Copy link
Author

justinmchase commented Aug 26, 2021

I haven't actually gone through the process yet of creating a package which dual supports both Deno and Node in the same codebase so I'm not totally sure but I think in this case if the files were all importing each other as .ts instead of .js then it would work fine through https://deno.land/x/yaml

Then next, when you transpile to an npm package then you would need to have the transpiler essentially change all of the imports from .ts to .js in the final version, I'm not sure if typescript does this automatically with the right flags or if there is another library that is needed...

Worth noting is that going through https://esm.sh/yaml to automatically transpile the published npm package into esm modules on the fly works.

So if I change my import in Deno from https://deno.land/x/yaml to https://esm.sh/yaml then it works, its just a bit of a sub-par experience going directly through deno land.

@eemeli
Copy link
Owner

eemeli commented Aug 26, 2021

Seems like this is a relevant upstream issue: microsoft/TypeScript#27481

Deno is being rather strange here, as changing the extensions in the source to .ts causes tsc to fail with this kind of error:

TS2691: An import path cannot end with a '.ts' extension. Consider importing './compose/composer.js' instead.

As I'd prefer not to include the built files in the source repo's development branch and keep the version tags in the main history, is there a way of making this work?

Tbh, the sense I get from Deno is that they've gone out of their way to making cross-compatibility with other environments difficult for library developers.

@justinmchase
Copy link
Author

The easiest possible fix for now may just be to put a link on the main README.md to tell Deno users to use https://esm.sh/yaml instead of deno.land, because it seems like that would work. In that case its just converting the published npm package into a cdn cached esm module.

And again, I haven't really done it yet and your project is big enough where its hard for me to just make a little PR and say "see here is how you do it". But it seems like if you make it for Deno and have all of the files in the original source form as Deno compatible code where you're importing the actual .ts files instead of the non-existant .js files then it would work for deno. Then when you're building for node you'd need a tool which essentially transpiles it all to js and alters the imports to point to the correct modules (now .js files), then pack that up push it to npm and then throw away all the intermediate build output. I see you're using a tool called rollup? I'm not familiar with that one and I think esm.sh is using esbuild.

Which appears to have the support for ts bundling maybe?
https://esbuild.github.io/api/#resolve-extensions

You might also be able to use swc?
https://swc.rs/

I'm not sure exactly. I'll have to experiment with something smaller.

Of course the ideal tool for this would be the typescript compiler tsc but as you pointed out they're taking a principled hard line stance against supporting importing .ts files for some inscrutable reason that frustrates nearly every one of their users.

Here, for example, is a classic comment in that thread:
microsoft/TypeScript#16577 (comment)

Which I believe is nearly every developers initial experience with ESM modules... its completely unintuitive but once you realize you have to add .js file extensions to everything then all of a sudden you think "well thats dumb but whatever...". That's the story of nodejs in a nutshell.

Contrast that with the experience of using Deno... Where the intuitive thing just works right away. And there's only one way to do it and you don't even need npm or a package manager, its just intuitive and easy. Yet it is definitely frustrating to also be compatible with nodejs... so one has to wonder who's fault that really is.

As someone who's been using Deno and dealing with these cross-compatibility issues I'll just just say its my perception that the reason why these challenges are happening is less because of Deno and more because of Node... Of course node is ubiquitous but many of these challenges came because members of the community just went ahead and solved some issues which became a defacto standard. Or like in this case, the standard was made purely with JS in mind and TS was ignored. Also making modules for the filesystem that weren't compabile with the web was a hugely bad idea. In my opinion Deno is coming in after all of this chaos with the advantage of hindsight and they have been taking a more purposeful approach with a more intentioned and reasoned decision making process, erring on the side of whats better and more correct than what happens in node which is more like what will work right now. Deno is the attempt to atone for the sins of its forebearers.

Adding an import in your typescript code to a file which doesn't even exist is an example of that. Where are the js files in your code? Why should you have to import .js files when all your files are .ts? Its weird. Its a very nodejs thing and we get accustomed to these janks but Deno has turned a number of these decisions over and it affects compatibility in difficult ways but mostly because the way it was done in nodejs is questionable. To really fix that problem in nodejs and typescript you have to essentially fix multiple foundational layers lower down in... in which case you end up with Deno!

I'm not a Deno team member and have no financial incentives to praise Deno but in my opinion Deno is the future and the more we can move that way even with the pain of trying to deal with backwards compatibility with node it will be worth it in the long haul. I am basically considering node a legacy platform at this point and Deno is its next-gen beta.

If you have solutions or feedback for the community of deno developers on how to more easily support cross-platform module development I think it would be very valuable for the community and would likely impact the future of web development.

Thank you for your efforts!

@eemeli
Copy link
Owner

eemeli commented Aug 26, 2021

I've previously participated in some of the discussion at denoland/deno#9569, but as you also mention, the Deno approach seems to be to figure out what works best in isolation of the rest of the JS ecosystem.

In this case, the mismatch actually has nothing to do with Node.js, but with TypeScript, and how Deno's loader for it differs from the one provided by Microsoft and used by everyone else.

As I mentioned previously, I don't really use Deno myself at all. I've tried previously to make sure that my code is Deno-compatible, but I'm not going to make it less compatible with tsc and the rest of TypeScript so that it works in Deno.

I would be willing to entertain a PR e.g. adding some note in the README and/or the docs site, but I continue to be sceptical about Deno's general lack of compatibility.

@justinmchase
Copy link
Author

justinmchase commented Aug 17, 2022

Since I had to re-read this and figure it out again, I just want to leave a note here about the actual solution. Perhaps something like below would be a nice addition to the main readme?

ESM

Its possible to load yaml using EcmaScript Module loading in Deno by importing through esm.js

export * as yaml from 'https://esm.sh/[email protected]';

@justinmchase
Copy link
Author

Or actually pulling from esm.sh seems to have problems with the latest version of Deno so an alternative is this:

import {
  parse,
  stringify,
} from "https://deno.land/[email protected]/encoding/yaml.ts";
export const YAML = { parse, stringify };

@jcayzac
Copy link

jcayzac commented Oct 31, 2022

@justinmchase that's not the same library.

This works just fine in Deno:

import YAML from 'https://esm.sh/[email protected]'
const { parse, stringify } = YAML

@jeff-hykin
Copy link

While ESM works, the module published on deno.land/x does not:

import Yaml from  "https://deno.land/x/[email protected]/src/index.ts"
> Uncaught TypeError: Module not found "https://deno.land/x/[email protected]/src/compose/composer.js".
    at https://deno.land/x/[email protected]/src/index.ts:1:26
    at async <anonymous>:1:50

(It should be looking for "https://deno.land/x/[email protected]/src/compose/composer.ts" but looks for js instead)

@jcayzac
Copy link

jcayzac commented Jun 8, 2024

import * as yaml from 'npm:[email protected]'

Or better yet,

deno add npm:[email protected]

Then just

import yaml from 'yaml'

Note that the standard library also already supports YAML, through @std/yaml:

deno add jsr:@std/yaml

@jeff-hykin
Copy link

Okay, If that's the official usage then you might want to remove the deno module since its broken

@justinmchase
Copy link
Author

I would agree with the previous comment, what's published to deno.land/x I would expect to work with Deno as a Deno compatible module.

@eemeli
Copy link
Owner

eemeli commented Jun 18, 2024

PRs welcome for adding a separate Deno endpoint and build that renames the imports and adds tests verifying that the result works in Deno.

@eemeli eemeli reopened this Jun 18, 2024
@jeff-hykin
Copy link

jeff-hykin commented Jul 6, 2024

Alright I made a generic tool for the renames using the tree-sitter parser. Haven't finished tayloring it to this codebase specifically, but I've got a hand-edited WIP branch over here to give you an idea of what the changes Look like.

In addition to .js->.ts

  • I had to mark whether something was a type export or a normal export
  • needed to prefix npm imports with "npm:"
  • needed to prefix node builtins with "node:"
  • needed to add import process from "node:process" instead of using the global process var
  • needed to change the "from 'yaml'" of the tests to actually import from the relative path

but after that it seems to pass tests.

I'm sure there's a way to do much of this with a deno.json, but I'm not a huge fan of deno.json

@eemeli
Copy link
Owner

eemeli commented Jul 7, 2024

  • I had to mark whether something was a type export or a normal export
  • needed to prefix node builtins with "node:"
  • needed to add import process from "node:process" instead of using the global process var

These sound like changes that could/should at least in part be applied directly to the source, rather than layered on for Deno specifically. As in, please file a separate PR for these.

  • needed to change the "from 'yaml'" of the tests to actually import from the relative path

Is Jest not used as the test runner for the proposed Deno build? At least in Node.js, it's handling the path rewrites for the test imports.

@jeff-hykin
Copy link

These sound like changes that could/should at least in part be applied directly to the source, rather than layered on for Deno specifically.

Great! Thats exactly what I was hoping to get some feedback on.

Is Jest not used as the test runner for the proposed Deno build?

Yeah, I have no idea how this works. Both I didnt realize that was a feature of Jest, and I'm not sure how that Jest feature ends up interacting with Deno. Thanks for that link. Whenever I look at this again, that will help me figure out what's going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants