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

Build as ESM and UMD libs #305

Merged
merged 8 commits into from
Sep 10, 2020
Merged

Build as ESM and UMD libs #305

merged 8 commits into from
Sep 10, 2020

Conversation

apisim
Copy link
Contributor

@apisim apisim commented Aug 17, 2020

The build now also produces libraries in ESM and UMD format. That makes it possible, for example, to use yaml-language-server in Monaco Editor with the YAML language server running as a worker in the browser.

Summary of the Changes

New Files

  • tsconfig.esm.json - configuration to compile the code as an ES module.
  • tsconfig.umd.json - configuration to compile the code in UMD format.

The output for both is a new directory /lib.

Modified Files

  • package.json - added to the existing build the compiling of the code to ESM and UMD libraries/modules; bumped @types/prettier to the latest version.
  • .gitignore - added lib/ so Git will ignore the untracked files in it.
  • src/languageservice/services/schemaRequestHandler.ts - replaced require with import.
  • src/languageservice/services/yamlFormatter.ts - replaced require with import. This makes it possible to compile with webpack and have the formatting work when running in the browser. There is another change described below.
  • yarn.lock - created by a "fresh" yarn install.

yamlFormatter.ts
yamlFormatter.ts now supports configuring indentation using the standard FormattingOptions from 'vscode-languageserver-types'. The existing code in server.ts can be seen as tightly coupled with prettier - tabWidth is specific to prettier.
Moreover, to configure the tab size when creating a Monaco Editor instance one has to use the standard tabSize; using tabWidth doesn't work.
Notice that this change shouldn't break any existing code that makes use of tabWidth (e.g. when running the YAML language server in something like VS Code).

Others

Notice that according to the npm-publish docs, "...All files in the package directory are included if no local .gitignore or .npmignore file exists. If both files exist and a file is ignored by .gitignore but not by .npmignore then it will be included". Therefore, my assumption is that the lib/ directory and its content will be published along with out/server.

@coveralls
Copy link

coveralls commented Aug 17, 2020

Coverage Status

Coverage increased (+0.4%) to 75.735% when pulling 81c2689 on apimastery:esm-and-umd-libs into 6bd6126 on redhat-developer:master.

@joshuawilson
Copy link
Member

@apisim Thanks for the PR. I think that updating all the dependencies may have caused the build error. I would like to see a massive version update like that done in a separate PR or at least commit as it has a high chance of breaking things.

@apisim
Copy link
Contributor Author

apisim commented Aug 17, 2020

@joshuawilson As far as I can tell, the Travis CI build was successful. From the two that failed, one is a 502 Bad Gateway error, which has nothing to do with the code, and the one for the windows build has no info in the raw log. Did something just time out? My builds on Windows run fine and the tests all run successfully.

I don't see a way to manually re-run the GitHub workflow actions. From what I've been reading, some workflow_dispatch configuration is needed?

I guess I'll have to push some trivial changes to trigger the workflow (when I get back to my laptop).

Thanks!

@joshuawilson
Copy link
Member

I was able to manually re-trigger them. We just got github actions setup, so I didn't know how to do it either.

@apisim
Copy link
Contributor Author

apisim commented Aug 17, 2020

Thanks, @joshuawilson!
I knew it wasn't problems with the code because the builds from the GitHub actions were all successful in my playground.

Copy link
Collaborator

@evidolob evidolob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apisim Could you update README.md?
For tsconfig* you can use extends, to put all common settings in one tsconfig.base.json and put different settings in proper tsconfig.esm.json and tsconfig.umd.json. And, I think, we do not need of old tsconfig.json

@evidolob
Copy link
Collaborator

@apisim Thanks for the PR. I think that updating all the dependencies may have caused the build error. I would like to see a massive version update like that done in a separate PR or at least commit as it has a high chance of breaking things.

+1 to @joshuawilson
It is better to make so huge dependency update in separate PR.

@apisim
Copy link
Contributor Author

apisim commented Aug 18, 2020

@apisim Could you update README.md?
For tsconfig* you can use extends, to put all common settings in one tsconfig.base.json and put different settings in proper tsconfig.esm.json and tsconfig.umd.json. And, I think, we do not need of old tsconfig.json

@evidolob Thank you for the review and the comments.
The build fails if I remove tsconfig.json. Here's a partial output:

$ yarn build
...
$ tsc -p .
error TS5057: Cannot find a tsconfig.json file at the specified directory: '.'.

Sure - I can extract common tsconfig settings into a tsconfig.base.json and have tsconfig.esm.json and tsconfig.umd.json inherit from it. Should I also make tsconfig.json extend it? tsconfig.json doesn't seem to have that many common configuration options with tsconfig.esm.json and tsconfig.umd.json...but let me know what you think.

Regarding updating README.md - what information in particular should be included or changed?

@apisim
Copy link
Contributor Author

apisim commented Aug 18, 2020

@apisim Thanks for the PR. I think that updating all the dependencies may have caused the build error. I would like to see a massive version update like that done in a separate PR or at least commit as it has a high chance of breaking things.

+1 to @joshuawilson
It is better to make so huge dependency update in separate PR.

OK - pushed a commit to back out the updates to the dependencies in yarn.lock. The only update I left is for @types/prettier - from "1.18.0" to the latest "2.0.2".
@joshuawilson @evidolob

@evidolob
Copy link
Collaborator

evidolob commented Aug 18, 2020

@evidolob Thank you for the review and the comments.
The build fails if I remove tsconfig.json. Here's a partial output:

$ yarn build
...
$ tsc -p .
error TS5057: Cannot find a tsconfig.json file at the specified directory: '.'.

To fix it you need to delete compile script, as it useless now.

Sure - I can extract common tsconfig settings into a tsconfig.base.json and have tsconfig.esm.json and tsconfig.umd.json inherit from it. Should I also make tsconfig.json extend it? tsconfig.json doesn't seem to have that many common configuration options with tsconfig.esm.json and tsconfig.umd.json...but let me know what you think.

I think we may have "default" package.json, which builds umd js files, and have additional tsconfig.esm.json which extends default and just brings changes which it need.

Regarding updating README.md - what information in particular should be included or changed?

At least we need to update getting-started section. And it would be nice to have some section which describes that now we have to type of modules, and how and when use them.

@apisim
Copy link
Contributor Author

apisim commented Aug 18, 2020

@evidolob Are you thinking of not producing and publishing out/server/** anymore but use lib/umd/** (or lib/esm/**) instead?

@evidolob
Copy link
Collaborator

I was thinking that it was your intention.
That why I ask to update README. In case if you want just to add more targets, we can keep everything as is.
Just extend tsconfig.json from tsconfig.esm.json and tsconfig.umd.json. And add to readme section about libraries.

@JPinkney
Copy link
Contributor

JPinkney commented Aug 19, 2020

Btw a lot of clients currently use out/server/** so if we got rid of that it would be a breaking change

E.g https://github.com/liuderchi/ide-yaml/blob/master/src/main.js#L21

@apisim
Copy link
Contributor Author

apisim commented Aug 19, 2020

I'm sorry for the confusion - no, my intention wasn't to replace out/server/**. I "...added to the existing build the compiling of the code to ESM and UMD libraries/modules".

Could you clarify, @evidolob, "...Just extend tsconfig.json from tsconfig.esm.json and tsconfig.umd.json"? Which will be the base, the "parent" and which will be the child[ren]?

I guess this "missed the train" for Release 0.10.0?

@evidolob
Copy link
Collaborator

tsconfig.json will be the 'base'(parent) ideally without modification, and tsconfig.esm.json and tsconfig.umd.json child.

As for release, I think you have time, till tomorrow, I do not have time to do it today.

@evidolob evidolob mentioned this pull request Aug 19, 2020
@joshuawilson
Copy link
Member

With these changes to the tsconfig name, we may need to set it in .eslintrc.js in the parserOptions section with something like "project": "./tsconfig.json" in there. I don't remember if we need it for each different tsconfig. I just want to make sure that the linter still has access to the tsconfig settings.

@apisim
Copy link
Contributor Author

apisim commented Aug 20, 2020

I'm not sure I understand, @joshuawilson - the original tsconfig.json didn't change and has the same (default) name.

On another note, @evidolob - it is a bit more complicated to update README.md with info on how to use the new ESM and UMD libraries. I'm currently using them in a monaco-yaml fork with the goal of merging all the changes in the fork back into the original monaco-yaml repo.
By using these libraries I am able to have redhat-developer/yaml-language-server running with Monaco Editor as a worker in the browser - no remote calls to a server are required like, for example, in case of Theia (if I properly understood Theia's architecture the last time I looked at it).

Given the transient nature of the monaco-yaml fork and the unknowns of when the changes will be merged into the original monaco-yaml repo (supposedly after the ESM and UMD libraries are published with yaml-language-server), I can't currently offer "how to use" instructions.

I don't want to be holding up Release 0.10.0. I think it might be best to close this PR. I will still probably submit a separate PR for the changes to schemaRequestHandler.ts and yamlFormatter.ts for future consideration.

Sincerely - thank you very much for considering the PR and for your time. Thanks!!

@evidolob
Copy link
Collaborator

@apisim Sorry, maybe I was not clear on update REDME request, I just want to inform users that we produce several different JS modules, and they may use this project not only like node modules.

As for release, we quite flexible, we may release 0.10.1 with this or any other your PR.

We, with @joshuawilson , wants to make release schedule more transparent, usually we make releases every 3 week, I will create some document which describe release process.

If you need to have some bug fix or feature released, don't hesitate to contact us, we will glad to help you.

@apisim
Copy link
Contributor Author

apisim commented Sep 3, 2020

If that's OK with you, I'd like to move forward with this pull request @joshuawilson @evidolob @JPinkney

I believe the comments so far were all addressed. Let me know, please, if there's some tweaking you think is needed or of anything else.

Thanks!

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side, I think both @evidolob and @joshuawilson are on PTO so I'm going to wait for them to get back before I merge

Copy link
Collaborator

@evidolob evidolob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
A few last things:

  • I think it would be better if we delete lib folder with clean script
  • It would be great if you add record about new libraries in to CHANGELOG.md

@apisim
Copy link
Contributor Author

apisim commented Sep 10, 2020

OK. I added a step to the clean script to clean up the /lib directory before a build; tested it when the /lib directory exists already or not. Also, I added a one-liner to CHANGELOG.md assuming the release would be 0.11.0.

Let me know if there's anything else.

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

Successfully merging this pull request may close these issues.

5 participants