search

w3c / yml2vocab

Generate RDFS vocabulary files from YAML
https://w3c.github.io/yml2vocab
Other
14 stars 5 forks source link

Upgrade all dependencies #20

Closed BigBlueHat closed 10 months ago

BigBlueHat commented 11 months ago

I wasn't able to run this locally on Node v18.18.0 due to conflicting peer dependencies in several of the TS related libraries, so I upgraded all the things. 😄

This also includes a .gitignore file to ignore node_modules/ for sanity. :wink:

Cheers! 🎩

iherman commented 11 months ago

~@BigBlueHat I am not sure what you did. All the changed files are in the dist subdirectory, which is generated via tsc. Did you make any changes on the original sources?~

Sorry, scrapping this; see https://github.com/w3c/yml2vocab/pull/20#issuecomment-1813937799

iherman commented 11 months ago

B.t.w., just for the fun, I have created a deno version of the same code; it is much cleaner to use deno instead of going through that tsc distribution. The only question is whether deno can be used with github actions easily; living with the saying "ain't broken, don't fix it", I did not want to go there.

iherman commented 11 months ago

Ah! Sorry, I think I realized where the real changes are: in the packages.json file. Scrap my comment in https://github.com/w3c/yml2vocab/pull/20#issuecomment-1812955862.

The question is: have you made a testing of our core vocabularies whether the updates are o.k. and the system work with the new version? Some of the upgrades in the npm packages may not be backwards compatible. I know I ran into problems, in another project, with the upgrade of the commander package (the one handling the CLI) which had some backward incompatible changes. Nothing major, but needed some minor adjustment on the codes. That is why I am wary...

We should find a way to avoid regenerating the js files for each PR when it is still in discussion. It is very misleading; those files should not be looked at during a review...

BigBlueHat commented 11 months ago

We should find a way to avoid regenerating the js files for each PR when it is still in discussion. It is very misleading; those files should not be looked at during a review...

I can remove the dist build from the PR and you can build them after the merge. Ideally, the dist/ directory wouldn't be in the repo at all, but I'm not familiar with how TypeScript projects handle that for distribution elsewhere (i.e. npm).

BigBlueHat commented 11 months ago

B.t.w., just for the fun, I have created a deno version of the same code; it is much cleaner to use deno instead of going through that tsc distribution. The only question is whether deno can be used with github actions easily; living with the saying "ain't broken, don't fix it", I did not want to go there.

Looks like there is a GitHub Action for deno: https://github.com/marketplace/actions/setup-deno

It's likely more a question of how you want to distribute this going forward. I'm pretty sure moving to deno would keep it off of npm (fwiw), but certainly avoiding the intermediate compilation step would be nice.

iherman commented 11 months ago

B.t.w., just for the fun, I have created a deno version of the same code; it is much cleaner to use deno instead of going through that tsc distribution. The only question is whether deno can be used with github actions easily; living with the saying "ain't broken, don't fix it", I did not want to go there.

Looks like there is a GitHub Action for deno: https://github.com/marketplace/actions/setup-deno

It's likely more a question of how you want to distribute this going forward. I'm pretty sure moving to deno would keep it off of npm (fwiw), but certainly avoiding the intermediate compilation step would be nice.

Much as I had some fun converting it into Deno, I am not sure we should change for now. We have two vocabularies in this WG, they converge to the final state, and I am a bit worried of the disturbance it would create to change the full publishing process by introducing Deno.

I think if we take the habit of doing the PRs on the sources only, and we (I?) should remember to run the distribution step before merging this, we are fine. I do not expect many changes. We could create a merge action in this repository to run the distribution process automatically, though... Interested?

iherman commented 10 months ago

@BigBlueHat you have not answered to this question:

Ah! Sorry, I think I realized where the real changes are: in the packages.json file. Scrap my comment in https://github.com/w3c/yml2vocab/pull/20#issuecomment-1812955862.

The question is: have you made a testing of our core vocabularies whether the updates are o.k. and the system work with the new version? Some of the upgrades in the npm packages may not be backwards compatible. I know I ran into problems, in another project, with the upgrade of the commander package (the one handling the CLI) which had some backward incompatible changes. Nothing major, but needed some minor adjustment on the codes. That is why I am wary...

The PR is pending on this...

iherman commented 10 months ago

@BigBlueHat,

I have cloned your branch locally to my machine, ran a fresh npm install and ran my simple test to see everything is o.k. The good news is that yes, it is.

However. Both your version and mine signals something different, which has been bothering me for a while. I get this:

16:15 yml2vocab-upgrade-all-dependencies> node dist/main.js -v local/tests/test.yml -t local/tests/test_template.html 
(node:60306) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

Running the deprecation flag gives this:

16:15 yml2vocab-upgrade-all-dependencies> node --trace-deprecation dist/main.js -v local/tests/test.yml -t local/tests/test_template.html
(node:60325) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
    at node:punycode:3:9
    at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:392:7)
    at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/realm:328:10)
    at loadBuiltinModule (node:internal/modules/helpers:101:7)
    at Module._load (node:internal/modules/cjs/loader:1001:17)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
    at Object.<anonymous> (/Users/ivan/Temp/yml2vocab-upgrade-all-dependencies/node_modules/psl/index.js:5:16)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)

Indeed, there is a pls module within node_modules which indeed imports punnycode. But that does not really help me. Any ideas?

BigBlueHat commented 10 months ago

@iherman sorry to not answer sooner! I've rerun the tests that build the examples, and it shows some interesting deviation. Mostly around language stuff. Most notably, the current JSON-LD output doesn't seem to be honoring the "@container": "@language" for some terms. Not sure why not, though. Just something that stood out as unexpected.

iherman commented 10 months ago

@iherman sorry to not answer sooner! I've rerun the tests that build the examples, and it shows some interesting deviation. Mostly around language stuff. Most notably, the current JSON-LD output doesn't seem to be honoring the "@container": "@language" for some terms. Not sure why not, though. Just something that stood out as unexpected.

Ah, you ran more tests than I did, which is good! Thanks.

From the top of my head, I have no idea what that can be... And it will be too late to look into this today for me. I will try to see tomorrow with the clone of your thing, unless you beat me into it...

iherman commented 10 months ago

Until this is solved, I will not refresh the npm package, ie, we should continue using the label field for now.

BigBlueHat commented 10 months ago

Indeed, there is a pls module within node_modules which indeed imports punnycode. But that does not really help me. Any ideas?

Sadly, I don't see those errors... Could you share the test documents you are using? Perhaps something in them is triggering a code path the other examples aren't trodding.

iherman commented 10 months ago

It does it on both the DI and the VCDM vocabularies!

It may be a local setup. As long as it does not interfere with our deployment, I do not care then.

(Unfortunately, I realized that the deno version does not work; jsdom is one of the rare npm packages that fail on deno. I have already raised an issue, maybe at some point...)

iherman commented 10 months ago

@BigBlueHat, coming back to https://github.com/w3c/yml2vocab/pull/20#issuecomment-1854139310 :

I have run the tools through the 'official' version as well as the cloned version with the updated package.json for both the VCDM and the DI vocabularies. I then ran a diff for all the generated ttl, jsonld, and html files, and there is no difference whatsoever.

So... if you experience a problem, it is probably not related to this PR but to the package itself. If you agree with this statement then, I believe, the right way forward is to (1) merge this PR and (2) you raise a separate bug report issue.

Is that o.k. with you?

BigBlueHat commented 10 months ago

@iherman I'm fine to merge this. The only "changes" to the output may not actually be related to the upgrades, but to some other change that happened along the way that wasn't followed by a commit to regenerate the examples...I'm not sure.

The most notable differences in the examples are a change from rdfs:Property to rdf:Property (which I think is either a correction or immaterial) and the addition of "@container": "@language" but the loss of the container-based expression in the output JSON-LD...which again may not be a bug related to this PR. Here are the container related lines, fwiw: https://github.com/w3c/yml2vocab/pull/20/commits/5831ed7ed59ea6471232de2c5ef12bc07155c4e2#diff-8be654d12d81cbc2fa4a21a1dacf5b382ca098b2ca1d5818317c4b87740e86a1R71-R86

I've no objections to merging this as it does get us onto more stable dependencies...but I did want to call out those changes in the output.

iherman commented 10 months ago

Hm. The rdfs:Property vs. rdf:Property was actually a bug at some point; I am too lazy to track down the exact date but I remember I had to update the script at some point. The current version generates rdf:Property, which is the right one. You may compare things against an outdated generated version...

I will merge this and then look at whether there is a problem or not with the @language.

iherman commented 10 months ago

Actually... I think I know what happened! Partially my mistake: the content of the example directory was not updated for a long long time, ie, all the generated ttl/json/etc. files are out of date... Hence the differences that you experienced.

I will add these generated files to .gitignore. It would avoid such discrepancies in the future.