Add build step for standalone version, resolves #842

[loilo]

This pull request resolves #842, adding a standalone build to the Prettier PHP plugin.

Build

The standalone.js in the root of the package is generated through running:

yarn run build-standalone

Usage

The standalone.js is "standalone" in the sense that it is bundled into one file with no Node.js-specific dependencies left. It does however not include Prettier itself. Prettier's standalone version has to be included on its own first (order matters):

<script src="https://unpkg.com/prettier/standalone.js"></script>
<script src="https://unpkg.com/@prettier/plugin-php/standalone.js"></script> <!-- does not exist yet -->

The plugin registers as a global variable named prettier_pluginPhp. Using Prettier with it look like this:

prettier.format(YOUR_CODE, {
  plugins: [prettier_pluginPhp],
  parser: ["php"]
});

Here's a demo.

Transformations

Things I instructed Rollup to do:

• Mark the prettier module as external, so essentially all require("prettier") are replaced with the prettier global variable.
• Add a shim for Node's assert module. I'm not sure why the strictEqual method should do nothing, but that's how it's done in Prettier.
• Add a shim for Buffer.isBuffer() (used by the php-parser dependency).
• Set process.arch to x32 (relevant for maximum float size).

ToDo

• Write tests
• Add documentation to readme

Considerations

• Name of the global?
  prettier_pluginPhp is not that catchy, suggestions are welcome.

  ✅ Resolved: Like Prettier's built-in plugins, the plugin is registered on the global prettierPlugins object as prettierPlugins.php.

• Minify?
  Prettier doesn't do it, therefore this pull request currently doesn't either.

  ✅ Resolved: Prettier does actually apply minification, not to their standalone.js, but to each of their plugins. This PR matches this behavior and minifies the standalone plugin.

• Add the generated standalone.js to the repo?
  Prettier does so.

  ❌ Rejected: I was wrong, Prettier actually does not.

• Is it reasonable to hardcode process.arch to x32?
  Doing a quick search, I found no way to detect a machine's architecture with browser-side JS with a reasonable amount of confidence. x32 should be a safe bet.

• Add documentation about how to use this plugin in bundlers?
  The standalone.js is not really suited for usage with bundlers. To avoid confusion, an additional section about using bundlers in the readme (i.e. "rewrite all prettier calls to prettier/standalone") could be reasonable.

  ✅ Resolved: No weird tips for bundler usage needed as this PR now also includes a pretty fool-proof way to integrate with bundlers.

[czosel]

Wow, this looks great! 💯

Concerning the questions:

• Name of the global: It doesn't sound too catchy indeed. Are there any conventions for this? @suchipi the name of the global for the lua plugin is generated from packd.now.sh, right?

• Minify: I agree - we can add this later if it becomes a requirement.

• Add built output to the repo? If I'm not mistaken, standalone.js in Prettier is not the actual bundle, but a module used as an entry point to generate the bundle. The actual bundle is generated in the dist folder which is not checked in to version control. I'd say we should do the same.

• process.arch: Good question. @ichiriac can you help us out with that? It seems that the relevant piece of code is this one: https://github.com/glayzzle/php-parser/blob/813caea94c5bb50370363cbc0843d721f218a3a7/src/lexer/numbers.js#L11

• I think the most important thing to document is how to include the plugin for usage in the browser. Usage with bundlers would be a nice addition though. This makes me wonder though: Could we somehow make it easier to bundle the plugin for in-browser usage? It would be nice if the rewrite step from prettier to prettier/standalone wouldn't be needed. In any case, this sounds like something we don't have to solve immediately.

[loilo]

Are there any conventions for this?

None that I know of. I'd be very thankful for a better suggestion though. 😁

If I'm not mistaken, standalone.js in Prettier is not the actual bundle, but a module used as an entry point to generate the bundle.

Absolutely. I looked that up but had already forgotten about it when I wrote down the pull request. 🙄

However putting the standalone file into dist proabably wouldn't work here.

Users need to be able to access the standalone build directly via @prettier/plugin-php/standalone, just as in Prettier, for consistency. This works for Prettier because their repo is so huge that they basically compile their whole npm package into the dist folder — I think we currently don't have good reasons for doing that.

So I'd stay with compiling the standalone bundle to the project root but add it to .gitignore.

Could we somehow make it easier to bundle the plugin for in-browser usage?

We could definitely make this a lot easier.

I could just add another Rollup config that differs slightly from the one in this PR:

• Instead of using the prettier global, it would rewrite require("prettier") to require("prettier/standalone"). This would also discharge the user of overriding the prettier module specifier globally, which gave me the shivers anyway.

  The user would still be in charge of importing prettier/standalone in their project (instead of just prettier), but that is a thing we should be able to clarify with documentation.

• Instead of generating a UMD bundle, it would create an ES module to export the plugin.

The resulting bundle would be listed in the package.json's browser field, which bundlers prefer over the main field when targeting browsers.

Writing it down like that, the effort should actually be pretty moderate. Would you like me to attach it to this PR directly?

[suchipi]

@czosel yeah- I assume it replaces characters that can't appear in a JS identifier with underscores (so @prettier/plugin-lua became _prettier_pluginLua, replacing the @ and /). I recommend prettierPluginPhp.

[loilo]

That's what I went with on first approach. Didn't like it that much, but I have to agree that mixed case is way worse.

[czosel]

So I'd stay with compiling the standalone bundle to the project root but add it to .gitignore.

👍

Writing it down like that, the effort should actually be pretty moderate. Would you like me to attach it to this PR directly?

Sounds great! 🙂

I recommend prettierPluginPhp

👍

[loilo]

I recommend prettierPluginPhp

👍

Done.

Write tests

Done as well.

I've approached this by renaming the test project to test-node and duplicating it as test-standalone with some necessary adjustments:

1. Exclude the markdown test — Prettier's standalone build does not include parsers, therefore the markdown parser is not available.
2. Add a global STANDALONE variable through the Jest config to achieve slight variations in the run_spec.js behavior.

I don't find either of these adjustments to be solved particularly elegant, so if a more experienced Jest user knows how to solve this in a nicer way, let me know!*

* Solving the first adjustment differently actually is a requirement, since using a neagive lookbehind assertion to exclude the markdown test breaks tests in Node.js v6.

Sounds great! 🙂

Alright, going to do that when the tests are fixed. 👍

[alexander-akait]

Awesome job! Something wrong in CI test 😕

[alexander-akait]

Any ideas how we can test this? Maybe run standalone.js on all tests and compare with snapshots too?

[loilo]

@evilebottnawi See my comment above — I've added tests already, also described how it works.

Failing CI is due to Node.js v6 not supporting RegEx backreferences (which I've used to exclude the markdown test from the standalone build). I haven't found an alternative way to exclude a test so far, would be nice if someone found a solution. 👍

[loilo]

Side note: I've actually discovered a better way for naming.

Prettier's very own parsers to be used with the standalone build (e.g. the parser-markdown.js) attach themselves to a global prettierPlugins object (or create that object if it doesn't exist).

This would be a pretty decent way for us as well, IMO. We'd go by prettierPlugins.php.

[loilo]

Okay, so I've fixed the tests by no longer excluding the markdown tests at all. I've rather included the markdown parser in the standalone test so this is now covered as well.

I had to update the snapshots of the markdown tests though since there was inconsistent behaviour between the builds: If no tabWidth is configured...

• ...the standalone build indents PHP code with the default width of the plugin (4 spaces)
• ...the Node.js build indents PHP code with the default width of Prettier (2 spaces)

This shoud definitely be streamlined, not sure which the desired behavior would be. This is a design decision. // cc @czosel @evilebottnawi

For now, I've changed the markdown test's config to explicitely use 4 spaces in every environment, this should however be reverted as soon as the behavior of both builds is aligned.

[alexander-akait]

@loilo strange maybe something wrong on prettier side?

[loilo]

It definitely seems to be something on Prettier's side (or in the official Markdown plugin). The Lua plugin shows the same behavior.

[alexander-akait]

/cc @suchipi

[czosel]

Both docs and code look great to me! 💯

I just have one more remark, because I misunderstood an earlier comment:

Side note: I've actually discovered a better way for naming.
Prettier's very own parsers to be used with the standalone build (e.g. the parser-markdown.js) attach themselves to a global prettierPlugins object (or create that object if it doesn't exist).
This would be a pretty decent way for us as well, IMO. We'd go by prettierPlugins.php.

Couldn't we do exactly the same as Prettier and attach ourselves to prettierPlugins? That would mean that usage of the PHP plugin in the Browser would look exactly the same as of Prettier core languages.

Otherwise, I'd prefer calling the global prettierPluginPhp (as originally suggested by @suchipi), because the dot in prettierPlugins.php looks a bit strange to me.

[loilo]

That's what I've done in 0bf6fa5. 👍

prettierPlugins.php was just my way to express "attaching ourselves to prettierPlugins".

prettierPlugins.php wouldn't be a valid variable name.

[czosel]

@loilo Ahh, that makes a lot of sense. Thanks for the enlightenment 😉

[loilo]

Thanks to the thought-provoking impulse from @evilebottnawi, we're now back to just a single UMD build that works universally in browsers and bundlers. ❤️

I've removed traces of the other builds and updated the docs accordingly.

[alexander-akait]

@loilo Great work! Anything left to finish?

[loilo]

I think we're good to go.

[czosel]

🎉

[alexander-akait]

/cc @suchipi Should we create issue about indent problem or you take care about this?

[czosel]

I just published v0.10.1 which contains the new and shiny bundle: https://unpkg.com/@prettier/plugin-php@0.10.1/standalone.js

Maybe someone is interested to try this with Prettier's playground? 😉

[loilo]

I was just about to say, what would you think of adding the standalone build to the Prettier playground as php (experimental)?

[suchipi]

@evilebottnawi could you create the issue please? I don't have all the context

[alexander-akait]

@suchipi I am also don't have, just review 😄

/cc @loilo maybe you can create issue?

[loilo]

Sure.

[loilo]

I find this really hard to nail down. Gotta sleep now and won't have time tomorrow, but if anyone's curious, here's what I found out so far:

• I can not reproduce the issue with the standalone build.

• It still occurs in the plugin's markdown test when removing the explicit tabWidth.

• Now the most magical observation: It does not occur in the plugin's standalone tests if the according test config is unwrapped from the projects (in the jest.config.js) and exported directly:

  module.exports = {
    displayName: "test-standalone",
    setupFiles: ["<rootDir>/tests_config/run_spec.js"],
    testRegex: "jsfmt\\.spec\\.js$|__tests__/.*\\.js$",
    snapshotSerializers: ["jest-snapshot-serializer-raw"],
    testEnvironment: "jsdom",
    globals: {
      STANDALONE: true
    }
  };

[alexander-akait]

@loilo Maybe you can create issue in our repo so as not to forget about it when someone has time

[loilo]

Yep, going to do that. Wasn't able to narrow it down myself.

[alexander-akait]

Anyway, thanks for good job and helping 👍

[loilo]

My pleasure. 🙂