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 Script alias #6237

Merged
merged 13 commits into from
Apr 15, 2024
Merged

Add Script alias #6237

merged 13 commits into from
Apr 15, 2024

Conversation

marklundin
Copy link
Member

@marklundin marklundin commented Apr 10, 2024

Having some discussions with @willeastcott, with the new ESM Script, we want to keep the default script boilerplate as clean as possible. This PR exports an alias for ScriptType named Script

import { Script } from 'playcanvas';

class Rotator extends Script {
  initialize() {
    ...
  }
}

UPDATE:
ScriptType has been added to the deprecation.js file
Improved dicunmentation for Script to include ESM example

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@marklundin marklundin requested a review from a team April 10, 2024 13:35
@kpal81xd
Copy link
Contributor

What was the reason for this?

mvaligursky
mvaligursky previously approved these changes Apr 10, 2024
Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

Looks good.
Should we rename ScriptType to Script in that module, and export it as ScriptType for compatibility?

@willeastcott
Copy link
Contributor

Should we rename ScriptType to Script in that module, and export it as ScriptType for compatibility?

Yeah, because otherwise, the API ref will continue to show ScriptType. The aliasing should be done in the backwards compatibility module.

@mvaligursky mvaligursky self-requested a review April 10, 2024 13:47
@marklundin
Copy link
Member Author

@willeastcott are we happy to simply rename the class and keep the script-type.js filename?

@mvaligursky
Copy link
Contributor

Rename filename as well, we keep those in sync.

@willeastcott
Copy link
Contributor

Maybe do:

  • script.js -> script-create.js (or script-register.js)
  • script-type.js -> script.js

@willeastcott
Copy link
Contributor

BTW, we should consider deprecating and hiding registerScript.

@mvaligursky
Copy link
Contributor

We also need to consider the impact in the Editor. Will the Editor be able to still use at least few previous engine versions? Do we need to add some type checking to it or similar? Maybe it all just works, but we need to check.

@marklundin
Copy link
Member Author

  • script.js -> script-create.js (or script-register.js)
  • script-type.js -> script.js

Makes sense, although strictly speaking this would be a breaking change right?

We also need to consider the impact in the Editor. Will the Editor be able to still use at least few previous engine versions? Do we need to add some type checking to it or similar? Maybe it all just works, but we need to check.

Yeah it should be fine, editor only uses ScriptType when parsing and pc.Script === pc.ScriptType

@marklundin
Copy link
Member Author

Actually maybe not a breaking change since we haven't yet published the ESM build . Mainly thinking about `import { Script } from 'playcanvas/framework/script/script-type.js'

@kpal81xd
Copy link
Contributor

Actually maybe not a breaking change since we haven't yet published the ESM build . Mainly thinking about `import { Script } from 'playcanvas/framework/script/script-type.js'

You would never do this normally it would be import { Script } from 'playcanvas'. Theoretically with the way the engine exports the unbundled version you can navigate through all the files to import just that file but its not guaranteed that file names wont change

@willeastcott willeastcott dismissed mvaligursky’s stale review April 10, 2024 21:56

PR needs more work based on feedback.

@marklundin
Copy link
Member Author

marklundin commented Apr 11, 2024

This has now been updated

  • Renamed script.js -> script-create.js
  • Renamed script-type.js -> script.js
  • Updated jsdoc references of {ScriptType} => {Script}

Verified with type docs...
image

*Note that now ScriptType does not exist, it does not generate a corresponding url. It would be great to retain the original, to indicated to users that it has been renamed

@marklundin marklundin requested a review from a team April 11, 2024 08:23
@marklundin
Copy link
Member Author

jsdocs update

image

@marklundin marklundin requested a review from willeastcott April 12, 2024 12:31
@marklundin
Copy link
Member Author

Any more updates on this @willeastcott? Keen to get this merged

Copy link
Contributor

@willeastcott willeastcott left a comment

Choose a reason for hiding this comment

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

Nice! LGTM.

@marklundin marklundin merged commit dd65f24 into main Apr 15, 2024
7 checks passed
marklundin added a commit that referenced this pull request May 13, 2024
marklundin added a commit that referenced this pull request May 13, 2024
slimbuck pushed a commit to slimbuck/engine that referenced this pull request May 20, 2024
@marklundin marklundin deleted the script-type-alias branch May 31, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants