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

feat: bundles for browser, esm, cjs #43

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

SettingDust
Copy link
Contributor

@SettingDust SettingDust commented Nov 30, 2022

fixes #35
fixes #42
closes #3
closes #5

Current problem:
The getNodeHtmlParser should be removed. But not. Of course it's not be called
waiting for evanw/esbuild#2706
But
is able to merge

there is yarn3 on my machine. Have to specific version avoiding update yarn lock version
@SettingDust SettingDust changed the title feat: bundles for browser, esm, cjs [WIP] feat: bundles for browser, esm, cjs Nov 30, 2022
@SettingDust SettingDust changed the title [WIP] feat: bundles for browser, esm, cjs feat: bundles for browser, esm, cjs Nov 30, 2022
@SettingDust SettingDust changed the title feat: bundles for browser, esm, cjs [WIP] feat: bundles for browser, esm, cjs Nov 30, 2022
@SettingDust SettingDust changed the title [WIP] feat: bundles for browser, esm, cjs feat: bundles for browser, esm, cjs Nov 30, 2022
src/nodes.ts Outdated Show resolved Hide resolved
build.ts Outdated Show resolved Hide resolved
build.ts Show resolved Hide resolved
@nonara
Copy link
Collaborator

nonara commented Nov 30, 2022

Thank you. I'll need to check it over a bit more thoroughly, but looks good so far.

Considering we're doing multiple builds, I would like to see some basic e2e testing for each of the built targets.

Could be as simple as load an HTML that covers all paths and verify that the output looks good. I believe there are some options for testing browser based DOM.

@nonara
Copy link
Collaborator

nonara commented Nov 30, 2022

Note to self (or @SettingDust if you want to):

Add support for this before release (in a separate commit):

Note to Self:

Rebase + squash into two separate commits before merge

@SettingDust
Copy link
Contributor Author

Thank you. I'll need to check it over a bit more thoroughly, but looks good so far.

Considering we're doing multiple builds, I would like to see some basic e2e testing for each of the built targets.

Could be as simple as load an HTML that covers all paths and verify that the output looks good. I believe there are some options for testing browser based DOM.

I haven't wrote e2e tests. But I'm interested.

@nonara
Copy link
Collaborator

nonara commented Dec 5, 2022

I haven't wrote e2e tests. But I'm interested.

Great! I will expedite review and getting it released when you have them ready. Browser test would be ideal, but not a dealbreaker if you don't have time for that part.

@SettingDust
Copy link
Contributor Author

SettingDust commented Dec 5, 2022

Great! I will expedite review and getting it released when you have them ready. Browser test would be ideal, but not a dealbreaker if you don't have time for that part.

A bit busy. ASAP

@SettingDust
Copy link
Contributor Author

SettingDust commented Dec 6, 2022

I have no idea what target should I test. ESM, CJS,Browser or Node, Browser?

EDIT:
Should we drop the outdated api for parsing html? Such as document stream or ActiveX

EDIT2:
I don't know is it e2e. But I think it's prove it works fine with native parser

@nonara
Copy link
Collaborator

nonara commented Dec 13, 2022

I reviewed everything closely. Great work! Thank you for adding TranslatorCollection tests as well.

What I meant by end-to-end is, because we have multiple targets being built, we need to do a basic test for each built target that ensures it can translate a reasonably complex HTML (one which covers most features) to the expected string.

How I often do it for something list ESM is create demo projects.

In this case, we can create projects for ESM and browser which are setup for ESM or to mimic browser import that have a main script which when executed, translates the HTML in an asset file and outputs it via console.log.

This way we can simple execute the project via child_process and check if the output matches our expected output and that it did not throw.

I created the skeleton structure for this in tests and pushed. If you want to try it and have any questions, let me know, and I'll be glad to help! Otherwise, I'll try to get to it when I can.

@SettingDust
Copy link
Contributor Author

SettingDust commented Dec 15, 2022

I don't quite understand how to get the results of the test to report properly like the top-level test. And is it necessary to build the main package once before testing? How else get the code to properly direct to the bundle needed in the real world?

@nonara
Copy link
Collaborator

nonara commented Jan 6, 2023

@SettingDust Sorry for the delay. Was out for the holidays! From here on out, I'll make sure I'm on this so we can push it through quickly!

I went ahead and made the end2end tests. Looks like we can do browser testing via jest-puppeteer, btw, so I made a skeleton for that.

Immediately, I'm seeing that we do have a problem on the esm build.

You can test it with debugger by node ./test/projects/esm/index.js and you'll see the issue. If you can take a look at it, let me know, and I'll help where I can.

@SettingDust
Copy link
Contributor Author

Caused by esbuild can't dynamic require. Looking into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants