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

Refactor to ESM #110

Closed
wants to merge 6 commits into from
Closed

Refactor to ESM #110

wants to merge 6 commits into from

Conversation

cossssmin
Copy link
Member

@cossssmin cossssmin commented Apr 19, 2024

This PR refactors the plugin to use ESM syntax. WIP.

I'll write a more detailed description once I make more progress.

The main goals here are:

  • ESM syntax/compatibility
  • updating the docs to Vitepress
  • updating the tests to Vitest
  • updating the release script

@cossssmin
Copy link
Member Author

@thewebartisan7 got most of the tests passing, just some related to props seem to fail and I'm not sure why.

I'm thinking something is broken in src/process-script.js:

597f76e (#110)

or in src/process-props.js:

597f76e (#110)

Can you take a look and help if possible? Thanks!

@cossssmin
Copy link
Member Author

@Scrum forgot to mention you, if there's anything you can think of regarding vm.createContext and ESM (or maybe there's something completely different, really not sure right now).

@thewebartisan7
Copy link
Collaborator

@thewebartisan7 got most of the tests passing, just some related to props seem to fail and I'm not sure why.

I'm thinking something is broken in src/process-script.js:

597f76e (#110)

or in src/process-props.js:

597f76e (#110)

Can you take a look and help if possible? Thanks!

Which error you get?

@thewebartisan7
Copy link
Collaborator

@Scrum forgot to mention you, if there's anything you can think of regarding vm.createContext and ESM (or maybe there's something completely different, really not sure right now).

Maybe this https://nodejs.org/api/vm.html#class-vmmodule ?

@cossssmin
Copy link
Member Author

Which error you get?

Either undefined vars like here:

https://github.com/posthtml/posthtml-components/actions/runs/8757901079/job/24037539339?pr=110#step:5:396

Or assertion errors, because it fails to remove the attribute that was passed as a prop, for example this one:

FAIL  test/test-locals.js > Must process props with custom options
AssertionError: expected '<div title="My Component Title"><h1>M…' to be '<div><h1>My Component Title</h1></div…' // Object.is equality

- Expected
+ Received

- <div><h1>My Component Title</h1></div><div><span>Body</span>Content-My slot prop</div><div>My prop via locals attribute</div>
+ <div title="My Component Title"><h1>My Component Title</h1></div><div><span>Body</span>Content-My slot prop</div><div>My prop via locals attribute</div>

Maybe this nodejs.org/api/vm.html#class-vmmodule ?

import vm from 'vm' fails too so does that mean we can't yet use vm to create the context and evaluate variables in ESM?

In this case I think moving away from vm is even more important in order to move the ecosystem to ESM and to try to make two of its most important plugins work in the browser (posthtml-expressions and this). There's also a discussion on posthtml-expressions about using something else than vm.

@thewebartisan7
Copy link
Collaborator

It seems main problem is process-script.js as you said.

I try to restore processScript to CommonJS and tests seems much better. You can also try:

  1. Restore process-script.js as before
  2. Rename file process-script.js to process-script.cjs
  3. Change import to require in process-props.js
    const processScript = require('./process-script.cjs');

Test result:

Screenshot 2024-04-21 at 19 28 41

I think error is just something related to expect of vite.

import vm from 'vm' fails too so does that mean we can't yet use vm to create the context and evaluate variables in ESM?

Honestly I don't have too much experience with VM contexts and I found that to run vm.Module is still experimental, see https://nodejs.org/api/vm.html#class-vmmodule (This feature is only available with the --experimental-vm-modules command flag enabled.).

Need more investigation into this, but consider to keep this as-is for now if there are not alternative.

About other issue in posthml-expression, I can't see how posthtml can run only in browser, am I missing something? What is the purpose of this? Is not the goal to just output HTML?

@cossssmin
Copy link
Member Author

I try to restore processScript to CommonJS and tests seems much better.

Yeah but if we are to move to ESM this won't work, will investigate further myself.

About other issue in posthml-expression, I can't see how posthtml can run only in browser, am I missing something? What is the purpose of this? Is not the goal to just output HTML?

You could transform HTML with it and its plugins right in the browser without running anything on the server like in a lambda function or similar. A more complex scenario would be being able to use Maizzle to render emails right in your browser, without the need of a server running Node. Basically, any tools that could transform HTML would benefit from this, you could even do drag and drop template builders for Maizzle and the like 👍

@thewebartisan7
Copy link
Collaborator

Yeah but if we are to move to ESM this won't work, will investigate further myself.

Although it is not ideal, I am almost certain that there is a way to configure it.

@cossssmin
Copy link
Member Author

OK, coming back to this I think we should merge the current dependency updates and we can deal with refactoring to ESM later.

node:vm should really be replaced with something that isn't Node.js-dependent when evaluating expressions in PostHTML, I'm currently looking at the following packages:

If it's ok with you, I'll go ahead and merge the dependency updates and also update the plugin to install fewer dependencies 👍

@cossssmin cossssmin closed this Jul 25, 2024
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