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

Errors resolving node dependencies when building with Angular #1441

Closed
kareldonk opened this issue Dec 5, 2024 · 22 comments
Closed

Errors resolving node dependencies when building with Angular #1441

kareldonk opened this issue Dec 5, 2024 · 22 comments

Comments

@kareldonk
Copy link

The 'modern' builders such as used in Angular have trouble resolving node dependencies that are included in markdownlint.

X [ERROR] Could not resolve "node:path"

    node_modules/markdownlint/lib/markdownlint.js:5:21:
      5 │ const path = require("node:path");
        ╵                      ~~~~~~~~~~~

  The package "node:path" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.


X [ERROR] Could not resolve "node:util"

    node_modules/markdownlint/lib/markdownlint.js:8:12:
      8 │ } = require("node:util");
        ╵             ~~~~~~~~~~~

  The package "node:util" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.


X [ERROR] Could not resolve "node:fs"

    node_modules/markdownlint/lib/markdownlint.js:731:35:
      731 │   const fs = options.fs || require("node:fs");
          ╵                                    ~~~~~~~~~

  The package "node:fs" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.


X [ERROR] Could not resolve "node:os"

    node_modules/markdownlint/lib/markdownlint.js:901:85:
      901 │ ..., helpers.expandTildePath(configExtends, require("node:os")), fs,
          ╵                                                     ~~~~~~~~~

  The package "node:os" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

There is an issue that describes the probem here: angular/angular-cli#27425

It works with the ng serve command because that uses a different way to bundle the files, but not with ng build.
According to the Angular devs, this is because the markdownlint "package does not expose its browser bundle to modern ESM bundlers."

It would be nice to have that support.

@DavidAnson
Copy link
Owner

I think this may be a different problem with the Angular builder. In the issue you link to, the comments suggest the relevant package exports different scripts for Node and for the browser. That's not something markdownlint does. (To be clear, there is a Webpack configuration that runs in the browser (see ./demo), but this is not exported, so it is not causing confusion. However, the existence of this does prove markdownlint can be successfully bundled to run in the browser.) The issue you report highlights imports with the "node:" prefix which is not universally supported by tooling in my experience. (I have to do something special for it for Webpack, for example.) My guess is this is what Angular is tripping up on, but again the fact that it works in some of their tools demonstrates there are not problems with how markdownlint is configured. Possible good news for your scenario is that I will publish a new version of markdownlint in a day or two which has been converted from CommonJS to ESM and that may help with the Angular problem. (However, the "node:" prefix is still in use as it is recommended by the Node team.) I will leave this issue open as a reminder, but everything I've seen so far suggests a bug in Angular tools which I'm not able to comment on without a better understanding.

@DavidAnson
Copy link
Owner

Thinking about this a little more, what I'm guessing may be going on is that Angular is trying to bundle a Node application with Node dependencies to run in the browser but they are NOT stubbing out the relevant Node dependencies. Instead, they expect every package to provide a browser export that does that for them. If so, I expect problems like this are fairly common with this approach.

As I wrote above, markdownlint CAN be bundled for the browser by stubbing out its Node dependencies and a proof/example/template for doing so can be found here:

@kareldonk
Copy link
Author

Yes it seems that when using their builder with the ng serve command, the Node dependencies are replaced with empty objects which in this case works and doesn't cause issues. I think that is also what you do in your demo project.

But it would be nice if you also provided an entry point in your library distribution specifically for browser use, instead of having to manually build this ourselves. One that would allow just importing the module in a JS/TS file and building like many other libraries have.

For now I'm going to see if there is a way to tell the Angular builder to stub our certain dependencies. Otherwise, I may have to do some manual work to include the required scripts like in your demo project.

@kareldonk
Copy link
Author

I managed to get it working in Angular using the following:

  1. Angular supports including external scripts during the build proces with a scripts property in angular.json. I added the required scripts there as follows:
 "scripts": [
  {
      "input": "./node_modules/markdownlint-micromark/micromark-browser.js",
      "inject": true,
      "bundleName": "markdownlint"
  },
  {
      "input": "./node_modules/markdownlint-micromark/micromark-html-browser.js",
      "inject": true,
      "bundleName": "markdownlint"
  },
  {
      "input": "./node_modules/markdownlint/demo/markdownlint-browser.js",
      "inject": true,
      "bundleName": "markdownlint"
  }
]
  1. In the typescript file I 'import' the API as follows:
const markdownlint = (<any>window).markdownlint.library;

then later in the code i can use it like:

markdownlint.sync(...);

This seems to work so far, however, the disadvantage of this is that the created JS bundle ("bundleName": "markdownlint") is injected always from the beginning in the index.html file which always loads the script even if it is not needed yet. Also, manually having to include the external scripts in angular.json also feels a bit hackey. Ideally I would have liked to just do the following in the typescript file at the top:

import markdownlint from 'markdownlint';

That actually works with ng serve but not when building using ng build because of the Node dependencies not being resolved.

@DavidAnson
Copy link
Owner

Great, thank you for the update! I've been thinking about this some more and I have an idea that might work for your scenario without forcing too many other weird changes. I don't think it will make the current release, but I'll probably play around with this soon.

@kareldonk
Copy link
Author

That's good news. I would appreciate it if you could let me know in this thread when the new solution is available so I can update my project with a better solution. I can also test it and give you feedback if needed.

DavidAnson added a commit that referenced this issue Dec 9, 2024
@DavidAnson
Copy link
Owner

@kareldonk, I have something for you to try in the branch browser. If you change your package.json dependency to "markdownlint": "DavidAnson/markdownlint#browser", clean, and run npm install, you should get the relevant changes. Note that my own webpack.config.mjs works more naturally as a result of these changes.

For reference: efefc6e

@kareldonk
Copy link
Author

kareldonk commented Dec 9, 2024

@DavidAnson I tried it and it works. Now i can put the following at the top of the TS file:

import * as markdownlint from 'markdownlint';
import * as markdownlintSync from 'markdownlint/sync';

Then later in the code I can do:

// Using exported types
const options: markdownlint.Options = {};

// Using the exported functions
markdownlint.applyFix(...);
markdownlintSync.lint(...);

I did have to change the settings in my Typescript tsconfig.json file as follows:

{
    "compilerOptions": {
        "module": "esnext",
        "moduleResolution": "bundler",
        ...
    },
}

Specifically, the moduleResolution had to be set to bundler (it used to be node), otherwise the markdownlint/sync import could not be resolved. I think this is because you're using some new Node export features in the way you are exporting the modules via package.json.

But now both Angular build methods work (ng build and ng serve).

Let me know if you need more info. Do you have an idea when this might be released?

@DavidAnson
Copy link
Owner

Great news! I just posted a release converting to ESM (that's why your imports changed), so I wouldn't normally release again for a little while. I might do this as a one off soon, depending on if there's fallout from the conversion.

@kareldonk
Copy link
Author

Thanks, I'd appreciate an update in this thread when you release it :)

@DavidAnson
Copy link
Owner

@kareldonk, could you please try again using next branch (via DavidAnson/markdownlint#next, remember to clean and re-install to get the changes)? This is staged to release and I'd like if you could verify it's still good AND also that types still import and behave correctly. I had to make a tweak to the exported .d.ts file for VS Code and it'd be nice to hear it still works for you. Once I get confirmation, I'll probably publish a new release in a day or two. Thank you!

@kareldonk
Copy link
Author

I tried it and when it comes to building and using the types it still works. One thing that I do want to mention is that, also with the previous test version above, in VSCode the editor is not able to automatically detect the markdownlint types. For example, if you are in a new TS file, and you have not imported anything yet from markdownlint, then typing the below line will not make it suggest an import from markdownlint:

const markdownlintOptions: Options = {};

On that line, VSCode will put red squiggly lines below Options, and when you hover over it to get Quick Fix suggestions, importing from markdownlint option does not show up. This does work for other libraries that export types.
This is a minor thing though, because you can just manually add the imports like I did above and it works after that.

There is another problem however. With this version, I am getting an exception when trying to lint the following text:

Testing 1 2 3

1.  Test
    -   Test

Oh no

Error:

Assertion: expected `startBufferIndex` to be `0`

This is thrown in the function sliceChunks(chunks, token) on the line:

ok(startBufferIndex === 0, "expected `startBufferIndex` to be `0`");

@DavidAnson
Copy link
Owner

Thank you for the update!

I'll see if I can reproduce the automatic import behavior you mention. I agree it is not a blocker, but I wonder if it is related to the extra level of indirection I have here because of subpath exports (not super common). I couldn't see another way without involving a tool to generate the declaration file. I'm curious if you know if this previously worked with markdownlint because all published versions so far have a single, self-contained declaration file.

Regarding the crash, I will definitely look into that. I can tell from the message that it's coming from the micromark parser which has not changed. So either that problem exists in published versions of markdownlint (possible, but seems unlikely given how simple your scenario is), or it got introduced somehow when I converted to ESM (but how?), or it got introduced when I added the browser export (unlikely in my mind).

Your example works fine in the current release (as tested using the browser demo link below), so I'm inclined to think this is new somehow and the ESM conversion is my guess.

https://dlaa.me/markdownlint/#%25mTesting%201%202%203%0A%0A1.%20%20Test%0A%20%20%20%20-%20%20%20Test%0A%0AOh%20no

@DavidAnson
Copy link
Owner

Oh, wait! The message you are seeing is a development-time message that goes away for release scenarios. It definitely could have existed all along because I know micromark has issues in this area since I've reported a few of them. :) It's probably not catastrophic and it has probably been there for a while. What's different now is that I used to need to bundle micromark for reuse because it is ESM and the library was CommonJS, but now that's no longer necessary and your workflow is giving you the development version. I can't tell much more from my phone right now, but if your tooling allows you to select production vs. development (as Webpack does), please try switching to production and see if this goes away. I'm not sure what to do about it (if anything), but it's great to understand this implication.

@DavidAnson
Copy link
Owner

Confirmed: The Assertion above is because you're running the "development" bits for micromark and it reproduces in main branch without the ESM or "browser" changes. Nothing for me to do about that other than recommend not doing that. :)

I'm going to look at doing the .d.ts file differently such that it's at least simpler.

@DavidAnson
Copy link
Owner

I am NOT able to reproduce the type issue you report:

user@HOST playground % cat test.ts 
const markdownlintOptions: Options = {};%
user@HOST playground % npm i DavidAnson/markdownlint#next

added 52 packages in 3s

38 packages are looking for funding
  run `npm fund` for details
user@HOST playground % code .
user@HOST playground % 

Then Options is red-underlined and Quick Fix, "Add import from markdownlint" produces:

import { Options } from "markdownlint";

const markdownlintOptions: Options = {};

If this is still happening for you, please let me know what to change about my steps to reproduce it.

Thank you!

@DavidAnson
Copy link
Owner

In branch inline-types, I've removed a single level of nesting from types.d.mts, but I'm not sure it accomplishes anything and am NOT currently thinking to keep that change. next branch is unchanged for now.

@kareldonk
Copy link
Author

You're right about the exception being thrown in the development build. I switched to production build for Angular and do not get the exception there.

As for importing the types automatically in VSCode, I tried it again with the next branch and now it works. I might have done something wrong before, not sure. Or maybe I had to restart VSCode.
In any case I confirmed it working now in my existing project and also when creating a new Typescript project.

So I guess everything is resolved now. :)

@DavidAnson
Copy link
Owner

@kareldonk, available in [email protected].

@kareldonk
Copy link
Author

Thanks!

@kareldonk
Copy link
Author

kareldonk commented Dec 16, 2024

@DavidAnson Thanks! Works in prod.

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

No branches or pull requests

2 participants