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

Fixes standalone mode & comment issues #29

Closed
wants to merge 7 commits into from
Closed

Conversation

rart
Copy link

@rart rart commented Dec 14, 2019

"Standalone mode" / Bundler-friendly

Using plugin-xml on web(pack) builds (in my case a react app), results in unusually long build times and compiler warnings. Looks like same issue as #28

Compiled with warnings.

./node_modules/prettier/index.js
Critical dependency: the request of a dependency is an expression

./node_modules/prettier/index.js
Critical dependency: the request of a dependency is an expression

./node_modules/prettier/index.js
Critical dependency: the request of a dependency is an expression

./node_modules/prettier/third-party.js
Critical dependency: the request of a dependency is an expression

./node_modules/prettier/parser-typescript.js
Module not found: Can't resolve '@microsoft/typescript-etw' in '/yada/yada/node_modules/prettier'

This PR switches print.js to require('prettier/standalone') instead of 'prettier' which works well in web builds and all (node) tests pass too.

Any objections to this approach? Alternatively there could be a standalone.js (same pattern that prettier itself follows) that does a whole different "build" of the plugin and somehow refactor print.js and plugin.js to minimize code duplication but seems unnecessary as this approach works fine.

Comment Issues

In addition to #13 and #14 I found another issue where a comment containing the < and > chars with a link inside gets utterly transformed.

Expected:

    <!--
    ~ Copyright (C) Some Corporation. All Rights Reserved.
    ~
    ~ Yada yada GNU General Public License
    ~ see <http://www.gnu.org/licenses/>.
    -->

Received:

    <http: //www.gnu.org/licenses="" />

I didn't realise about #30 until I had finished this but when I ran the changes on #30 with my XML updates - i.e. the ones on this PR - the tests didn't pass. Tests all pass with the changes on this PR as well as with the test XML on #30.

➜  prettier-plugin-xml git:(master) ✗ yarn test
yarn run v1.19.2
$ jest
 PASS  test/comment.test.js
 PASS  test/node.test.js
 PASS  test/attrs.test.js
 PASS  test/leaf.test.js
 PASS  test/arrays.test.js
 PASS  test/cdata.test.js
 PASS  test/location.test.js
 PASS  test/doctype.test.js

Test Suites: 8 passed, 8 total
Tests:       21 passed, 21 total
Snapshots:   0 total
Time:        1.256s
Ran all test suites.
✨  Done in 2.28s.

Switches print.js to require('prettier/standalone') which works in web builds. Running (node) tests all pass.
@rart rart mentioned this pull request Dec 14, 2019
… issue where comment containing a comment containing a string like `<http://www.gnu.org/licenses/>` gets utterly transformed.
Added .idea to ignores (intelliJ/webstorm editor project files).
@rart rart changed the title [28] Standalone mode Standalone mode & Comment issues Dec 14, 2019
@rart rart changed the title Standalone mode & Comment issues Standalone mode, comment issues & !doctype preserve Dec 16, 2019
This reverts commit f230921. Reverting since, including a new expression on the tagPattern regex changes indexes used by `parse` function breaking some of the parsing. This was breaking the `leaf` tests only apparently. Needs more careful inclusion to not break some of the other use cases.
@rart rart changed the title Standalone mode, comment issues & !doctype preserve Fixes standalone mode & comment issues Dec 16, 2019
@rart
Copy link
Author

rart commented Dec 18, 2019

@kddeisz wondering if you have any thoughts on this?

Also, once approved, what's the release cycle like?

@kddnewton
Copy link
Member

@rart a bit slammed at work but I'll take a look this weekend. I can release immediately after merging.

@ghost
Copy link

ghost commented Dec 23, 2019

@kddeisz This PR renders #30 moot. If this is accepted, #30 can be closed.

@ghost
Copy link

ghost commented Jan 14, 2020

@kddeisz Can you please make a point of looking at this PR? I've had to remove this plugin from all of my (Maven) projects, as it's breaking my builds as it is currently.

@kddnewton
Copy link
Member

Half of this is out of date now because of all the parser changes in #39. If you wouldn't mind retesting the standalone issues and reporting back to #28 then we can continue the discussion there

@kddnewton kddnewton closed this Jan 19, 2020
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.

None yet

2 participants