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

Enable development without running the browser extension #20

Merged
merged 37 commits into from
Feb 18, 2021
Merged

Conversation

akirk
Copy link
Collaborator

@akirk akirk commented Feb 3, 2021

This Pull Request adds the ability to use HAR files for local development.

By recording an HAR file using a browser's developer tools (usually there is something like "Save All as HAR" that can be used for that), either after having visited a supported site, or after an export took place, this HAR file can then be used for continued, local (=offline) development.

Specifically, this adds a CLI script export-from-har.js that accepts a HAR file as argument and then runs the business logic of exporting the site. Contrary to running in the browser and thus making HTTP requests to the real URLs, the requests are satisfied (if possible) by using the specified HAR file.

This also allows quick testing with different sites by swapping out the locally saved HAR files.

To achieve this workflow, we have created a package fetch-from-har that implements a polyfill for fetch() (which is not available in node anyway and would need its own polyfill) which is initialized with the HAR file to load the data from.

Testing Instructions

In order to bridge the compatibility gap between ES6 used in the extension, and common JS used in node scripts, we're using webpack, hence during development you need to keep a npm run watch running in a tab.

The script can then be executed with either of these:

// extract media manager files and the communities-blog-app:
$ npm run extract-wix my.har -- -a 14bcded7-0066-7c35-14d7-466cb3f09103,media-manager
// extract media manager files and the communities-blog-app:
$ npm run extract-wix my.har -- -a media-manager
// if you call the node file directly, you don't have to do the -- trick
$ node build/export-from-har.js wix my.har -a 14bcded7-0066-7c35-14d7-466cb3f09103

For the moment, it'll output the exported WXR to the console.

As a side-product of the first implementation using fetch-mock (see #17), this also adds a jest test case.

@akirk
Copy link
Collaborator Author

akirk commented Feb 5, 2021

Update: After making good progress today (see the edited description of this PR), I have run into a problem with @wordpress/blocks:

(node:27463) UnhandledPromiseRejectionWarning: Error: Block type 'core/paragraph' is not registered.
    at sanitizeBlockAttributes (webpack://free-as-in-speech/./node_modules/@wordpress/blocks/build-module/api/utils.js?:255:11)

I wonder why this works in the browser extension (although I have to admit I haven't succeeded in exporting a WXR even from the extension yet) but not in node. Maybe I need to register the core blocks first?

@akirk
Copy link
Collaborator Author

akirk commented Feb 5, 2021

When providing no configuration data, we already get an empty WXR:

$ node build/export-from-har.js ~/Downloads/wix.har
<?xml version="1.0"?>
<!-- This is a WordPress eXtended RSS file generated by the "Free as in Speech" browser extension, as an export of your site. -->
<!-- It contains information about your site's posts, pages, comments, categories, and other content. -->
<!-- You may use this file to transfer that content from one site to another. -->
<!-- This file is not intended to serve as a complete backup of your site. -->
<!--  -->
<!-- To import this information into a WordPress site follow these steps: -->
<!-- 1. Log in to that site as an administrator. -->
<!-- 2. Go to Tools: Import in the WordPress admin panel. -->
<!-- 3. Install the "WordPress" importer from the list. -->
<!-- 4. Activate & Run Importer. -->
<!-- 5. Upload this file using the form provided on that page. -->
<!-- 6. You will first be asked to map the authors in this export file to users -->
<!--    on the site. For each author, you may choose to map to an -->
<!--    existing user on the site or to create a new user. -->
<!-- 7. WordPress will then import each of the posts, pages, comments, categories, etc. -->
<!--    contained in this file into your site. -->
<rss version="2.0" xmlns:excerpt="http://wordpress.org/export/1.2/excerpt/" xmlns:content="http://purl.org/rss/1.0/modules/content/" xmlns:wfw="http://wellformedweb.org/CommentAPI/" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:wp="http://wordpress.org/export/1.2/">
	<channel>
		<pubDate>Fri, 05 Feb 2021 13:10:05 GMT</pubDate>
		<wp:wxr_version>1.2</wp:wxr_version>
	</channel>
</rss>

@akirk
Copy link
Collaborator Author

akirk commented Feb 8, 2021

ERROR in ./node_modules/jsdom/lib/jsdom/utils.js 152:2-27
Module not found: Error: Can't resolve 'canvas' in '/home/runner/work/free-as-in-speech/free-as-in-speech/node_modules/jsdom/lib/jsdom'

This error that's preventing the test from running through successfully is not really an error, more of an "accepted error" for optional dependencies, see webpack/webpack#7713 (comment). Unfortunately, just addiing canvas as another dependency doesn't work either because of this error:

ERROR in ./node_modules/canvas/build/Release/canvas.node 1:0
Module parse failed: Unexpected character '�' (1:0)

As a counter-measure, I have simply excluded the error from the webpack output. Not the nicest workaround.

@akirk
Copy link
Collaborator Author

akirk commented Feb 11, 2021

Ok, now that I have circumvented the error message, I am finalizing the CLI syntax:

// extract media manager files and the communities-blog-app:
$ npm run extract-wix my.har -- -a 14bcded7-0066-7c35-14d7-466cb3f09103,media-manager
// extract media manager files and the communities-blog-app:
$ npm run extract-wix my.har -- -a media-manager
// if you call the node file directly, you don't have to do the -- trick
$ node build/export-from-har.js wix my.har -a 14bcded7-0066-7c35-14d7-466cb3f09103

@akirk akirk marked this pull request as ready for review February 11, 2021 12:07
@yoavf
Copy link
Collaborator

yoavf commented Feb 11, 2021

Testing Instructions

Does it make sense to include some HAR files in this PR for testing?

@akirk
Copy link
Collaborator Author

akirk commented Feb 11, 2021

I suppose it'd only make sense if we had a test extractor that would extract those specifically provided files there. I'll work on that.

Copy link
Owner

@pento pento left a comment

Choose a reason for hiding this comment

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

This is looking really good! It was super easy to use, and I can see how easy it'll make development, when we don't have to constantly be waiting on the browser to refresh.

I left a couple of comments on things that I think could be improved. Additionally, I like @yoavf's suggestion of including some HAR files to use: it seems like we could safely generate a bunch of HAR files, replace tokens and such with placeholders, and commit them.

It's probably worth having two sets of HAR file tests: the package should have its own Jest tests to confirm that it's functioning correctly, independent of any functionality built against Wix (or other services). Additionally, for Wix specifically, I think this is a good opportunity to set up integration tests (in same vein as Gutenberg).

Finally, I noticed that running npm i caused some changes to package-lock.json. Could you double check that?

bin/export-from-har.js Outdated Show resolved Hide resolved
bin/export-from-har.js Outdated Show resolved Hide resolved
@akirk
Copy link
Collaborator Author

akirk commented Feb 15, 2021

With the integration tests I am running into CommonJS vs ECMAScript module problems again. The code works when I run the webpack'ed script via npm run extract-wix test/integration/wix/fixtures/wix-basic.har but the same code invoked via jest fails.

It does make a difference if I run

test/integration/wix work-offline $ jest --config ../../../jest.config.js fetch-from-wix-har.test.js
./source/services/wix/index.js:4
import WXR from '@wordpress/wxr';
^^^^^^

SyntaxError: Cannot use import statement outside a module

  3 | const fetchFromHAR = require( 'fetch-from-har' );
  4 | const getWXRFromWixHAR = require( '../../../bin/lib/get-wxr-from-wix-har' );
> 5 | const wixServices = require( '../../../source/services/wix' );
    |                     ^
  6 | 
  7 | test( 'wix-basic', () => {
  8 | 	const inputHAR = fs.readFileSync(

vs.

$ jest test/integration/wix/fetch-from-wix-har.test.js
./packages/wxr/src/index.js:4
import moment from 'moment';
^^^^^^

SyntaxError: Cannot use import statement outside a module

  2 |  * WordPress dependencies
  3 |  */
> 4 | import WXR from '@wordpress/wxr';
    | ^
  5 | 
  6 | /**
  7 |  * Internal dependencies

Jest has some support for ECMAScript modules (see source/services/wix/extractors/communities-blog-app/test/index.js) but something prevents it from applying it to the module.

@akirk
Copy link
Collaborator Author

akirk commented Feb 16, 2021

The next step I took is to try and get out of mixing Common JS and ECMAScript modules and I converted everything to the latter. @pento has been using its syntax already in the extension (import vs require()).

A benefit of this is that the CLI tool can be run without webpack, so node bin/export-from-har.js wix test/integration/wix/fixtures/wix-basic.har just works (I suppose with a decently recent node version, I have 12.20.1).

The negative aspect of this is, that building the extension is currently broken because I am stuck with converting the usage of require.resolve() which exists both in webpack.config.js and in bin/packages/build.js.

While this now increases the size and impact of this PR a lot, I feel it makes sense to go with the more modern ESM, so I'll proceed. Maybe there is a way to split this PR but first I'd like to get it running.

@akirk
Copy link
Collaborator Author

akirk commented Feb 16, 2021

Ok, I finally made some real progress here. So now almost everything runs again, I am just running into some lint issues that I'll fix tomorrow.

  • npm run build works again by just using webpack directly. The browser extension is built properly and works.
  • npm run test-unit works fine as well (which now also runs the integration test that can be run directly via jest test/integration/wix/fetch-from-wix-har.test.js).

Running node bin/export-from-har.js wix test/integration/wix/fixtures/wix-basic.har was working but currently throws a

import { createBlock, serialize } from '@wordpress/blocks';
                      ^^^^^^^^^
SyntaxError: Named export 'serialize' not found. The requested module '@wordpress/blocks' is a CommonJS module, which may not support all module.exports as named exports.

I'll look at this tomorrow.

@akirk
Copy link
Collaborator Author

akirk commented Feb 17, 2021

After some friendly help from @sirreal (thanks!), I reverted course (I kept the ESM approach in this branch for reference) and converted everything to Common JS. This means we no longer transpile, thus I removed .babelrc. So now, this works:

  • Exporting through the extension (most important).
  • Running the CLI directly without webpack: node bin/export-from-har.js wix test/integration/wix/fixtures/wix-basic.har
  • The unit and integration tests, executed via npm run test

This change to CommonJS causes a conflict with #23, since it still uses ESM syntax.

Copy link
Owner

@pento pento left a comment

Choose a reason for hiding this comment

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

Wow, this has turned into a huge project! 🙂

I've caught up with the conversations around the CJS vs ESM package loading syntax. While I have a personal preference for ESM, it's certainly not something that stands in the way of switching to using CJS, instead. I'd much prefer us to be able to use this tool, than to try and find workarounds for the oddities of build tools.

Thanks for taking care of the other issues I noted. I'm still seeing changes to package-lock.json when I run npm i on this branch: could you check if it's happening to you?

Aside from that, go ahead get this PR merged, so we can start using it properly. 🎉

I'll take care of rebasing #23 once that's done.

@akirk
Copy link
Collaborator Author

akirk commented Feb 18, 2021

While I have a personal preference for ESM

Me too, this is why I went down that road first. But turns out node just isn't ready yet and/or there is no agreement yet on how to load ESM from modules.

I'm still seeing changes to package-lock.json when I run npm i on this branch: could you check if it's happening to you?

It didn't happen for me when you first posted but happens now, I just opted not to take care of this until this PR was ready to merge. I'll update it.

Aside from that, go ahead get this PR merged, so we can start using it properly. 🎉

Thanks! Will do!

@akirk akirk merged commit 57b680b into main Feb 18, 2021
@akirk akirk deleted the work-offline branch February 18, 2021 09:39
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

3 participants