informatics-isi-edu / ermrestjs

ERMrest client library in JavaScript
Apache License 2.0
4 stars 3 forks source link

Upgrade markdown-it related dependencies #930

Closed RFSH closed 2 years ago

RFSH commented 2 years ago

In this PR,

The only changed behavior because of this update was related to how attributes are working in combination with markdown table. Everything else should continue to work as before.

I mainly created this PR to run test cases in CI and also start a conversation about dependencies in ermrestjs. I'm still not sure what's the best way to handle dependencies in ermrestjs. Currently, ermrestjs test cases are running in nodejs environment and we're using npm to install the dependencies. This also helps us with finding vulnerabilities through Github (we're listing them in package.json and Github is using this list to match with the list of known issues). But, to make sure chaise can also include the dependencies we're keeping the browser-compatible version of dependencies in the vendor folder. As you can see, this is not really ideal. Ideally when we upgrade chaise and ermrestjs, ermrestjs will be using proper build tools to create a bundle that chaise can use. With that, we can ensure the dependencies that we're using while testing and in chaise are the same.

Given this, before merging this PR, I should make sure this branch of ermrestjs works properly with chaise as well.

jrchudy commented 2 years ago

The only change to syntax is needing an extra \n character before the attribute block now?

RFSH commented 2 years ago

@jrchudy yes, that's the only thing that I noticed.

RFSH commented 2 years ago

Given that we decided to change how we're managing the dependencies in ermrestjs, I made a lot of other changes that are not related just to markdown-it. So for the sake of history I'm going to close this PR and create a new one that includes all the changes.