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

Add support for multi-config JSEP (mostly backward compatible) #272

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

Conversation

6utt3rfly
Copy link
Collaborator

@6utt3rfly 6utt3rfly commented Nov 18, 2024

Before this change, JSEP parse was constructing a new instance of the JSEP class to encapsulate the expression and index as it was parsed, and the JSEP configuration was static on the class.

Since JSEP parsing is synchronous, separate calls to parse can safely reset the expression and index instead of creating a new instance. Doing so allows the configuration to be moved into the JSEP instance, so that separate configurations can be used for different jsep parsing usages. The parse method then resets the instance's expression and index rather than constructing a new instance. This allows JSEP to functionally maintain backward compatibility despite the structural change.

This commit essentially adds instance() (with default clearConfig() setup), and defaultConfig() methods. All static accesses were replaced with this.

Specific things to note:

  • JSEP still exports the parse function with the instance's properties and methods for configuration
  • all static methods and properties have been moved to the instance (even constants and methods that don't access instance properties, so they can still be used by plugins). To keep the static constants available on each instance without forcing users to now access the prototype, they get assigned in the constructor
  • the export is still the instance's parse function, with all methods and properties accessed through a Proxy (rather than copying all properties and binding all methods)
  • the Jsep class export is replaced with the jsep instance export so that { Jsep } = require('jsep') still works as before (unless user was calling (new Jsep('expr')).parse())
  • because the ternary plugin is included by default, it gets specially registered once, so that defaultConfig can include the default plugin as well
  • fix removeAllBinaryOps not clearing right_associative

fixes #211
fixes #28

BREAKING: changes JSEP internals to be a class instance per config, rather than per parse. Jsep export is now the combined parse function & class instance instead of the Jsep class. so usages that were directly using the Jsep class (new Jsep(expression)) will have to update to either Jsep(expression) or Jsep.parse(expression). Most applications should be backward compatible without any changes to code

Before this change, JSEP `parse` was constructing a new instance of the JSEP class to encapsulate the expression and index as it was parsed, and the JSEP configuration was static on the class.

Since JSEP parsing is synchronous, separate calls to parse can safely reset the expression and index instead of creating a new instance. Doing so allows the configuration to be moved into the JSEP instance, so that separate configurations can be used for different jsep parsing usages. The `parse` method then resets the instance's expression and index rather than constructing a new instance. This allows JSEP to functionally maintain backward compatibility despite the structural change.

This commit essentially adds `instance()` (with default `clearConfig()` setup), and `defaultConfig()` methods. All static accesses were replaced with `this`.

Specific things to note:
- JSEP still exports the parse function with the instance's properties and methods for configuration
- all static methods and properties have been moved to the instance (even constants and methods that don't access instance properties, so they can still be used by plugins). To keep the static constants available on each instance without forcing users to now access the prototype, they get assigned in the constructor
- the export is still the instance's parse function, with all methods and properties accessed through a Proxy (rather than copying all properties and binding all methods)
- the Jsep class export is replaced with the jsep instance export so that `{ Jsep } = require('jsep')` still works as before (unless user was calling (new Jsep('expr')).parse())
- because the ternary plugin is included by default, it gets specially registered once, so that `defaultConfig` can include the default plugin as well
- fix `removeAllBinaryOps` not clearing `right_associative`

fixes #211

BREAKING: changes JSEP internals to be a class instance per config, rather than per parse. `Jsep` export is now the combined parse function & class instance instead of the Jsep class. so usages that were directly using the Jsep class (`new Jsep(expression)`) will have update to either `Jsep(expression)` or `Jsep.parse(expression)`. Most applications should be backward compatible without any changes to code
@6utt3rfly
Copy link
Collaborator Author

6utt3rfly commented Nov 18, 2024

@EricSmekens @LeaVerou - I'm open to feedback and suggestions with this. I tried to keep it as backward-compatible as possible, so that most users should not have any issues with upgrading, but I still marked it as a breaking (major) version update.

Since it's breaking, we could potentially add other changes though, like:

  1. default to empty config by default?
  2. Maybe change the export to be just the class so that users have to call new Jsep() and then configure it (even with defaultConfig(), minus the ternary plugin? Or the default config could even be moved into a plugin?
  3. I debated using Object.defineProperties() to make the constants read-only on the instance. I also wanted to keep them accessible to the plugins without having to update them as well, which is why they're on the instance.
  4. The typings file could be updated to remove the namespace (see Update typings for TypeScript 5.6 #269 )

@EricSmekens
Copy link
Owner

Great change! Will definitely support some more complex use cases of jsep users.

I'm all for upgrading a major version, and the 4 points you mention, are nice to add as well, but it is also fine to keep this change smaller and up another major version whenever we have those change ready. I will leave that decision up to you.

Based on that decision, maybe we can add a small Upgrade from 1.x.x to 2.x.x instruction list. (In readme or release notes)

@6utt3rfly
Copy link
Collaborator Author

This PR as it is now, should be backward compatible for the majority of users. The complication is in the JSEP package public API that returns a function, and properties/methods for configuration, or even a static class + parse method:

import jsep from 'jsep'; // function + config properties/methods
jsep.addUnaryOp('@');
jsep('1 + 1');

// or:
import { Jsep } from 'jsep'; // Class! Could technically call `new Jsep('exp').parse()`
Jsep.addUnaryOp('@');
Jsep.parse('1 + 1');

// or:
const jsep = require('jsep').default;

// or:
const { Jsep } = require('jsep');

// or:
<script type="module">
  import jsep from '/PATH/TO/jsep.min.js';
</script>

// or:
<script src="/PATH/TO/jsep.iife.min.js"></script>

Maybe we should keep this PR mostly as is, but if we decide on an API moving forward, then we could add some deprecation warnings and update the readme? I think the annoying thing right now is exporting a function with properties on it. This PR switched from assigning each class property to the function in a loop, to using a Proxy

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.

Multiple instances of jsep with different operators Set parser to vanilla state
2 participants