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

Implement Site.Prototype methods in ES6 syntax #2280 #2356

Merged
merged 6 commits into from Sep 3, 2023

Conversation

jmestxr
Copy link
Contributor

@jmestxr jmestxr commented Aug 17, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Resolves #2280 according to solution proposed in #979

Overview of changes:

  • Refactor Site.prototype methods implemented in Site/index.ts using ES6 class syntax

Anything you'd like to highlight/discuss:

Testing instructions:

npm run test

Proposed commit message: (wrap lines at 72 characters)

Refactor Site.prototype methods in ES6 syntax


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@tlylt
Copy link
Contributor

tlylt commented Aug 19, 2023

solution proposed in #979

Which solution are you referring to?

@jmestxr
Copy link
Contributor Author

jmestxr commented Aug 21, 2023

solution proposed in #979

Which solution are you referring to?

I was referring to this comment: #979 (comment)
using this syntax: rebuildAffectedSourceFiles = delay(this._rebuildAffectedSourceFiles, 1000);

ESlint doesn't show any parsing errors for me so I assumed it was updated alr.

@tlylt
Copy link
Contributor

tlylt commented Aug 25, 2023

solution proposed in #979

Which solution are you referring to?

I was referring to this comment: #979 (comment) using this syntax: rebuildAffectedSourceFiles = delay(this._rebuildAffectedSourceFiles, 1000);

ESlint doesn't show any parsing errors for me so I assumed it was updated alr.

I see, so because class fields proposal is accepted in TC39 hence there is no more syntax error complained by Eslint?
Could you investigate the browser support for class fields since it's only accepted last year (2022)?

I briefly searched it and found that class fields seem to not work in IE at all and have about ~92% support globally
https://caniuse.com/?search=class%20field

It's not a big issue, but good to find out more before we make this change.

@jmestxr jmestxr marked this pull request as draft August 25, 2023 06:59
@jmestxr
Copy link
Contributor Author

jmestxr commented Aug 30, 2023

I see, so because class fields proposal is accepted in TC39 hence there is no more syntax error complained by Eslint? Could you investigate the browser support for class fields since it's only accepted last year (2022)?

I briefly searched it and found that class fields seem to not work in IE at all and have about ~92% support globally https://caniuse.com/?search=class%20field

It's not a big issue, but good to find out more before we make this change.

I did some search and the browser support for both constructor and class fields are similar, so logically speaking, MarkBind should be able to continue supporting the browsers that we are currently supporting.

Javascript class is not supported in IE at all: https://caniuse.com/?search=class. So anyway, our current MarkBind is not supported in IE at all

@jmestxr jmestxr marked this pull request as ready for review September 2, 2023 07:04
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #2356 (b18857a) into master (34493cc) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2356      +/-   ##
==========================================
+ Coverage   45.11%   45.16%   +0.05%     
==========================================
  Files         123      123              
  Lines        5165     5159       -6     
  Branches     1086     1086              
==========================================
  Hits         2330     2330              
+ Misses       2516     2510       -6     
  Partials      319      319              
Files Changed Coverage Δ
packages/core/src/Site/index.ts 29.75% <100.00%> (+0.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -26,7 +26,7 @@ const addHandler = (site, onePagePath) => (filePath) => {
}
Promise.resolve('').then(async () => {
if (site.isFilepathAPage(filePath) || site.isDependencyOfPage(filePath)) {
return site.rebuildSourceFiles(filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, maybe there's something wrong here. I'm not sure why we are rebuilding all source files when one file is added/removed. Anyhow, it's not related to this PR.

* Build/copy assets that are specified in filePaths
* @param filePaths a single path or an array of paths corresponding to the assets to build
*/
buildAsset = delay(this._buildMultipleAssets as () => Bluebird<unknown>, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might cause a type problem in the future when serveUtil.js is migrated to TypeScript, but should be ok for now.

@tlylt tlylt added this to the v5.0.3 milestone Sep 2, 2023
@tlylt tlylt merged commit d1adec8 into MarkBind:master Sep 3, 2023
8 checks passed
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.

Refactor methods implemented using Site.prototype
2 participants