jsenv / importmap-node-module

Generate importmap for node_modules
33 stars 4 forks source link

Support for TypeScript decorators? #37

Closed DannyMoerkerke closed 3 years ago

DannyMoerkerke commented 3 years ago

I have a TypeScript project that I would like to generate an import map for. When I run the script to generate the map I get the following error:

SyntaxError: This experimental syntax requires enabling one of the following parser plugin(s): 'decorators-legacy, decorators' 

My project contains LitElement which contains TypeScript decorators. Is this even supported and if so, how can I get this working?

I suspect this needs a Babel decorators plugin as a dependency but correct me if I'm wrong.

dmail commented 3 years ago

Not in the current version.

As you suspect you would need babel plugin for decorator in your dependencies and enable them during importmap generation.

It should be easy to make this possible so I'll publish a new version shortly. Once the version will be published on npm you will be able to do something like:

import { getImportMapFromProjectFiles, writeImportMapFile } from "@jsenv/node-module-import-map"

const projectDirectoryUrl = new URL("./", import.meta.url)

await writeImportMapFile(
  [
    getImportMapFromProjectFiles({
      projectDirectoryUrl,
+     babelPlugins: ['@babel/plugin-syntax-decorators']
    }),
  ],
  {
    projectDirectoryUrl,
    importMapFileRelativeUrl: "./project.importmap",
  },
)

Assuming you have @babel/plugin-syntax-decorators in your devDependencies or some of your dependencies have it, it will work.

I keep you informed.

DannyMoerkerke commented 3 years ago

Thank you for your quick response!

If you need help with this I'll be happy to contribute of course.

dmail commented 3 years ago

If you need help with this I'll be happy to contribute of course.

Glad to hear! Let me put into words what I have in mind to support your use case. Then if you feel ready to do it, I'll let you do the pull request :)

dmail commented 3 years ago

Support decorators while generating importmap

The problem

Knowing that:

  1. To generate importmap, js files are parsed.
  2. This parsing is done by babel with some plugins.
  3. The decorator plugin is not enabled.
  4. It's not possible to enable decorator babel plugin from outside.

As a result if a js file contains decorators, an error is thrown and importmap are not generated. Moreover it's not possible to fix because there is no way to enable the babel plugin manually.

Solution A

Add "decorators" to the list of plugins at https://github.com/jsenv/jsenv-node-module-import-map/blob/bb4e23b138bb5a272361f32a768f996fad8ea669/src/internal/from-js/parseSpecifiersFromFile.js#L26-L28

Solution B (prefered)

Add a new option like "ecmaProposals" in jsFilesParsingOptions.

https://github.com/jsenv/jsenv-node-module-import-map/blob/bb4e23b138bb5a272361f32a768f996fad8ea669/src/internal/from-js/parseSpecifiersFromFile.js#L17-L21

This option would be enabled by default. When enabled it would enable all plugins required to support ecma proposals listed in babel parser documentation.

Screenshot of babel doc ![image](https://user-images.githubusercontent.com/443639/122757009-6c12d400-d297-11eb-98d0-877c9c42fb5b.png)

Expectations

  1. The pull request must allow to generate importmap when js files contains decorator(s).

  2. The pull request must add a test trying to generate importmap for js files containing a decorator. The following test file can be used as example https://github.com/jsenv/jsenv-node-module-import-map/blob/bb4e23b138bb5a272361f32a768f996fad8ea669/test/getImportMapFromProjectFiles/core/basic/basic.test.js#L1

    I recommend writing the test first, see it fail, then implement solution A or B to see the test being fixed. A test can be executed directly with the node command:

    > node ./test/getImportMapFromProjectFiles/core/basic/basic.test.js

You want to handle this? Feel free to ask any additional information you may need

DannyMoerkerke commented 3 years ago

I'm on it!

How do the names of the plugins map to the names that are provided in the plugins array that is passed to parser.parse()?

For example, I see jsx and flow, but the actual plugins are named plugin-transform-react-jsx and preset-flow.

dmail commented 3 years ago

According to what I see in the babel source code, the babel-parser plugins are different from the usual babel plugins.

For example. this is what @babel/plugin-proposal-decorators does under the hood:

https://github.com/babel/babel/blob/9cbe283bb8f0d879130db814198e7d7bca86dceb/packages/babel-plugin-proposal-decorators/src/index.js#L58

So to enable decorators, you would do this:

plugins: ["decorators"]

And not

plugins: ['@babel/plugin-proposal-decorators']

To sum up, I assume the list of plugin name to enable is the one from https://babeljs.io/docs/en/babel-parser#ecmascript-proposalshttpsgithubcombabelproposals. In other words: "asyncDoExpressions", "classStaticBlock", ...

DannyMoerkerke commented 3 years ago

I made the changes to /src/internal/from-js/parseSpecifiersFromFile.js and ran npm run build which generates /dist/commonjs/jsenv_node_moudle_importmap.cjs.

When I import it with import { getImportMapFromProjectFiles, writeImportMapFile } from "../jsenv-node-module-import-map/dist/commonjs/jsenv_node_module_importmap.cjs";, I get the error:

require() of ES modules is not supported

How can I test this?

dmail commented 3 years ago

You cannot import .cjs file with import keyword. You must use require to do that.

But you don't have to! The build generating cjs file is only for backward compatibility in case someone cannot use import and only has access to require. The source files can be used right away: import { whatever } from "@jsenv/node-module-import-map"

dmail commented 3 years ago

If you need you can use https://nodejs.org/dist/latest-v14.x/docs/api/module.html#module_module_createrequire_filename

DannyMoerkerke commented 3 years ago

Ok got it, I'm now importing from index.js and that works, but now I get the following error:

[Error: ENOENT: no such file or directory, open '/Users/danny/web/fe-lit/node_modules/@material/package.json'] 

The thing is that only each subfolder inside /node_modules/@material contains a package.json file.

Am I doing something wrong?

Here's the script I use:

import { getImportMapFromProjectFiles, writeImportMapFile } from "../jsenv-node-module-import-map/index.js";

const projectDirectoryUrl = new URL("./", import.meta.url)

await writeImportMapFile(
  [
    getImportMapFromProjectFiles({
      projectDirectoryUrl,
    }),
  ],
  {
    projectDirectoryUrl,
    importMapFileRelativeUrl: "./project.importmap",
  },
)
dmail commented 3 years ago

The script is correct, can you explain what you are trying to do in details?

If a package.json is missing either you should add it manually or run npm install.

DannyMoerkerke commented 3 years ago

I cloned the project, added the plugins to parseSpecifiersFromFile.js and I'm running the script from a sibling folder where my other project is located. I already ran npm install again but the issue is that there is no package.json in /node_modules/@material because this is a parent folder for many dependencies like @material/mwc-button, @material/mwc-textfield etc. so there simply is no package.json in the @material folder. There are in fact many npm dependencies with this structure.

dmail commented 3 years ago

I see, this should not be a problem.

Jsenv tries to read a package.json when:

  1. It reads the project root dependencies. So it would happen if projectDirectoryUrl is node_modules/@material
  2. The root package.json or a nested one contains a reference to @material in dependencies/devDependencies.

According to your info I assume no package.json references @material as it's not a real package. I don't see why jsenv would try to read that package.json if we are not in case 1 or 2. 🤔

If you can give me steps to reproduce from scratch I will be able to tell you what is going on when coming back to code on Monday 🤠

dmail commented 3 years ago

The script generates importmap by reading package.json recursively. If the project directory does not have a package.json the script will fail.

So I wonder: Are you running the script from node_modules/@material?

DannyMoerkerke commented 3 years ago

No, I'm running it from the root directory of the project.

DannyMoerkerke commented 3 years ago

When I remove all @material dependencies and run the script I get an almost empty import map even though there are still some dependencies left in package.json:

{
  "imports": {
    "defie": "./index.ts"
  },
  "scopes": {}
}
dmail commented 3 years ago

getImportMapFromProjectFiles first generates all mappings for dependencies found in package.json files. Then it will read js files and remove all mappings not used in the files. This second step can be disabled by disabling treeshakeMappings. If you disable treeshakeMappings, how is your importmap file?

    getImportMapFromProjectFiles({
      projectDirectoryUrl,
+     treeshakeMappings: false
    }),
DannyMoerkerke commented 3 years ago

With treeshakeMappings: false an importmap file is generated, but I still have to remove all @material/... dependencies. I then notice many errors that these dependencies are not found although these are present in node_modules but that's probably because they're removed from package.json.

My project is written in TypeScript and when I run the script to generate the import map from the root of the project I also get errors that the compiled JavaScript files are not found. That is because the compiled files are placed in the dist folder.

When I change the projectDirectoryUrl to const projectDirectoryUrl = new URL("./dist", import.meta.url) these errors go away.

I have to refer to these compiled files including the .js extension in the TypeScript source files, otherwise they're not found by the browser. So I have imports like import './src/components/form-component.js for example.

Could that have anything to do with it?

dmail commented 3 years ago

const projectDirectoryUrl = new URL("./dist", import.meta.url)

Looks great: browser will use dist/ files so importmap should be generated from dist/ 👍

Your use case reminds me of https://github.com/jsenv/jsenv-node-module-import-map/issues/20#issuecomment-693579068. In this situation @trusktr is doing two things before generating importmap:

  1. npm run build: create the dist directory and its content
  2. ln -sf ../node_modules dist/node_modules: create a symbolic link to node_modules into the dist directory

The project is visible at https://crimson-tick-0pebs672.ws-eu10.gitpod.io/

Doing a symlink of the node_modules looks like the solution we are looking for 🤔 . Can you try ?

DannyMoerkerke commented 3 years ago

I'm actually copying package.json and package-lock.json to the dist folder after the build and then run npm i there so node_modules is created inside dist, because this is what my client needed. The error about the missing package.json inside @material remains unfortunately.

dmail commented 3 years ago

Can you share the sources files? Or a minimal file structure to reproduce?

dmail commented 3 years ago

I would like to understand how the node_modules/@material file structure gets created and why there is no package.json in it.

DannyMoerkerke commented 3 years ago

I'm currently on holidays but will get back to you asap!

dmail commented 3 years ago

The API of this package has evolved and it would be difficult to resume the work you started. For this reason I have made a PR myself to fix this issue: https://github.com/jsenv/importmap-node-module/pull/41. For the record, the commit with the fix is https://github.com/jsenv/importmap-node-module/pull/41/commits/77dc0a1ea010b6be6309fa2a3f41461edb05cd04.

I am about to merge this pull request that will publish a new version with the fix: 2.2.0.

I have added a test case for project using decorators, the file structure is visible at https://github.com/jsenv/importmap-node-module/tree/77dc0a1ea010b6be6309fa2a3f41461edb05cd04/test/js_import/decorators/root.

As you can see there is a babel config file, if you don't already have one, it is required to enable the decorators plugin.

I am closing the PR because the decorator issue should be fixed. However I am still curious to know more about your file structure and how it results into ENOENT.

DannyMoerkerke commented 3 years ago

Ok, I will update to 2.2.0 when it's released and try again. If it still doesn't work I will provide an example project. I used this in code for a client that I'm not allowed to make public of course

dmail commented 3 years ago

Wonderful.

And if that does not work, I am almost certain that with an example project I will be able to help you.