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

Added browser support #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucafabbian
Copy link

This branch provides an experimental release of sparql-engine for the browser.

The release file is stored under browser/sparqlEngine.js. In the same folder, you will also find an example.html file, a 1:1 porting of the already existing N3 example, which proves that the library works and is usable.

The release file is generated through the rollup.js builder, with a minor hack documented in the rollup.config.mjs configuration file.

I still have to port the sparql-engine test suite to the browser, I'm going to do it as soon as possible.

Copy link
Owner

@Callidon Callidon left a comment

Choose a reason for hiding this comment

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

Hi, Thank you for your contribution. You have several issues with this PR that needs to be fixed before merging it

  • All tests jobs are broken because you use a rollup version that is not compatible with the range of Node version supported by the sparql-engine package.
  • I am not very happy about introducing a bundler for an "added browser support". Since we are using the typescript compiler, aren't they any method for injecting the missing poylfills using the TS compiler instead?
  • The browser/sparqlEngine.js file is a non-minified bundle of the whole package, committed as a regular file. It is a big no go for me. If you wish to continue with the bundler approach, you need to remove these kind of files, and make their generation dynamic in the Github action release job.

@lucafabbian
Copy link
Author

Hi :)

  1. I did not realize tests were run also against node 10 and 12. Is it really mandatory to support such outdated versions? They already reached their end of life, even considering the LTS. Moreover, it's just for testing. If I understand it right, the actual file will keep the compatibility with those versions. And even if they would not, I do not see why anyone with a node of 5 years ago would be interested in upgrading sparql-engine.

  2. I do not think so. Typescript is not as powerful as Rollup, and many other libraries use them in conjunction. Rollup just works, and it may become even more useful for future tweaks.
    For example, while I was testing the BIND bug, I had to compile version 7.0, which had issues due to old N3 being not suitable for the browser. However, with just few lines of code, I replaced the misworking file with some manipulation magic and everything went right:


const patchedN3 = `
// Expose submodules
var exports = module.exports = {
  Lexer:        require('./lib/N3Lexer'),
  Parser:       require('./lib/N3Parser'),
  Writer:       require('./lib/N3Writer'),
  Store:        require('./lib/N3Store'),
  StreamParser: require('./lib/N3StreamParser'),
  StreamWriter: require('./lib/N3StreamWriter'),
  Util:         require('./lib/N3Util'),
};
`
function n3Patcher() {
  return {
    name: 'n3-patcher',
    resolveId(source, importer) {
      if (source.endsWith('N3.js')) {
        return path.resolve(path.dirname(importer), source);
      }
    },
    load(id) {
      if (id.endsWith('N3.js')) {
        return patchedN3;
      }
    }
  };
}

This would be impossible with just Typescript.

  1. Can I just gitignore the current browser/sparqlEngine.js ? The file should already be available in npm, because the npm run build scripts generates it. If you wish, I may also generate a minified version with Terser. However I do not think it's really needed: keep in mind that most setups do include a minification of imported node modules, and even automatic CDN services such as jsdelivr automatically provide a minified version.

If you wish, I may also add an action for building and pushing the result to a gh-pages branch, to allow a live preview on sparql-engine served by Github Pages.

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.

2 participants