-
Notifications
You must be signed in to change notification settings - Fork 585
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
Babel plugin changes #4938
Babel plugin changes #4938
Conversation
26cda48
to
963fcdc
Compare
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @github-actions[bot]. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @github-actions[bot]. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
1 similar comment
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @github-actions[bot]. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
// Add the following plugins: | ||
plugins: [ | ||
'@realm/babel-plugin', | ||
['@babel/plugin-proposal-decorators', { legacy: true }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that we should also publish a @realm/babel-config
which would check / add these plugins, to simplify consumption. Perhaps not right away, but eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a nice idea! I can look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created #5009 to track it
!types.isClassProperty(classPropertyNode) || | ||
classPropertyNode.optional | ||
) { | ||
throw new Error(getLinkingObjectsError("Properties of type LinkingObjects cannot be optional")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇 for preconditions + errors ... Perhaps we're missing some elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah probably – I actually meant to ask your opinion on whether this is overkill or not? i.e. should we just rely on the type definitions?
I think throwing an explicit error is probably better though, as sometimes people will ignore TS errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to send this to babel somehow, in a way that wouldn't make it completely unusable if a property is misconfigured, but simply error through to the bundler. I don't know what's available for babel plugins to report errors or warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that is a good idea – I'll do some research.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this should be a throw
, as if you replace it with e.g. a console.warn
, you then end up with Property 'xxx' of type 'linking objects' cannot be nullable.
– so you are specifying an invalid schema.
However if you use throw path.buildCodeFrameError(errorMsg)
, you get a better error message which shows the source of the error, so I will switch to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't really think of any other good preconditions. The type argument for List/Dictionary/Set is optional and if they supply more than 1 type argument for some reason it will still work as intended... doesn't really seem worth checking. Can you think of anywhere else?
1a0c9c6
to
5190b15
Compare
@kraenhansen I made the changes we talked about yesterday:
|
@@ -28,6 +28,7 @@ Based on Realm JS v10.21.1: See changelog below for details on enhancements and | |||
``` | |||
### Enhancements | |||
* Small improvement to performance by caching JSI property String object [#4863](https://github.com/realm/realm-js/pull/4863) | |||
* Added new package `@realm/babel-plugin` to enable definining your Realm models using standard Typescript syntax [#4938](https://github.com/realm/realm-js/pull/4938) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial announcement of the plugin can be done here. For the future I suggest that plugin's package has its own changelog.
|
||
## Introduction | ||
|
||
The Realm Babel plugin enables defining your Realm models using standard Typescript syntax - no need to define a separate schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Realm Babel plugin enables defining your Realm models using standard Typescript syntax - no need to define a separate schema. | |
The Realm Babel plugin enables defining your Realm models using standard TypeScript syntax - no need to define a separate schema. |
// Add the following plugins: | ||
plugins: [ | ||
'@realm/babel-plugin', | ||
['@babel/plugin-proposal-decorators', { legacy: true }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created #5009 to track it
(d) => | ||
d.node && | ||
isRealmDecoratorWithName(d.node.expression, name) && | ||
// ((types.isIdentifier(d.node.expression) && d.node.expression.name === name) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be removed?
d.node && | ||
types.isCallExpression(d.node.expression) && | ||
isRealmDecoratorWithName(d.node.expression.callee, name) && | ||
// types.isIdentifier(d.node.expression.callee) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be removed?
What, How & Why?
This PR contains all additional changes I've made to Kraen's Babel Plugin branch, except the template app changes which are in a separate PR, because they can't be merged yet.
I've also created a separate PR containing the entire Babel plugin pointing to
v11
if we'd prefer to review it all in one.☑️ ToDos
Compatibility
label is updated or copied from previous entryCOMPATIBILITY.md
package.json
s (if updating internal packages)Breaking
label has been applied or is not necessaryIf this PR adds or changes public API's: