Closed chris--jones closed 2 years ago
I should add that I created two new package.json script entries for building the output:
npm run build
will generate index.js in the source folder (for running unit tests)the output minified js is significantly smaller than it used to be which mildly concerns me.
I think I figured this out - it's because it's targeting newer browsers so doesn't need the shims. I think I can adjust the config for this to support older browsers, but given the significant reduction in size you might want to consider dropping support for older browsers or releasing a larger version alongside the smaller one depending on what level of browser support people want.
I'm converting this to draft so I can add babel back and include configuration for supported browser versions.
Hello @chris--jones, I think this is great. Like your previous WAVE export pull request, this is again one of the best pull requests I've ever received.
I think we don't need shims anymore, but if you think that generating both files would be better, then I'm OK. Alternatively, maybe whoever needs old browser support can use ~3.0 as I'll release this as 4.0.
I'd be happy to accept this one when you feel like nothing more is needed.
Hey @ozdemirburak - appreciate the feedback. I was assuming the older browser support was required because of the prior inclusion of babel. Looking at the transpiled code it looks like any modern browser version released over the last 5 years should be ok.
In case you decide you want older browser support, it should be a simple as adding
@rollup/plugin-babel
, @babel/core
and @babel/preset-env
packages and updating rollup.config.js
to include:
plugins: [typescript(), babel({
babelHelpers: 'bundled',
presets: ['@babel/env']
})]
Fixes #23
I tried to do some minor cleanup as well namely fixing some eslint complaints, but ended up having to replace gulp with rollup in order to get the bundling happening (all the corresponding gulp plugins were abandoned/archived). Along with replacing istanbul with nyc and removing coveralls (possibly abandoned?) this seems to have resolved all of the package vulnerabilities as well!
I made the dependency of options one-way with characters so it is now passed in to getCharacters rather than have getOptions mutate characters (non-obvious side-effect) - I renamed the old getCharacters to getMappedCharacters for clarity.
I tried to make each module consistent in how it uses options, so they all assume the options have been processed to add in defaults if needed.
Note: I also removed babel as it seems the transpilation from typescript is already covering browser compatibility, but for some reason the output minified js is significantly smaller than it used to be which mildly concerns me.