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

Update the packages for some bugs' fixtures #2172

Merged
merged 1 commit into from Apr 8, 2019
Merged

Update the packages for some bugs' fixtures #2172

merged 1 commit into from Apr 8, 2019

Conversation

ghost
Copy link

@ghost ghost commented Apr 8, 2019

  1. IMPORTANT Fix:Update the 'marked' package, because there's an existing BUG
    reported by @rvagg, and this is already fixed in its v0.6.2.
    Ref:Markdown parsing problem #2119.
    Fixture:[Bug] We have to use '\' to add the '(' markedjs/marked#1434.

  2. A general upgration of the two packages:
    a) require-dir: 1.0.0 => 1.2.0
    b) semver: 5.6.0 => 6.0.0 (Fix a technical bug).


All syntaxes passed with unit tests

@ghost ghost requested review from rvagg and fhemberger April 8, 2019 01:44
@ghost ghost changed the title Update the packages Update the packages for some bugs' fixtures Apr 8, 2019
@fhemberger
Copy link
Contributor

fhemberger commented Apr 8, 2019

The marked fix seems to work. However, I have a strange effect on my local build (the live version is fine).

The current master with DEFAULT_LOCALE=en npm run build produces the following output for me (example: locale/en/about/index.md):

<p><code>`</code>javascript
const http = require(&#39;http&#39;);</p>
<p>const hostname = &#39;127.0.0.1&#39;;
const port = 3000;</p>
<p>const server = http.createServer((req, res) =&gt; {
  res.statusCode = 200;
  res.setHeader(&#39;Content-Type&#39;, &#39;text/plain&#39;);
  res.end(&#39;Hello World\n&#39;);
});</p>
<p>server.listen(port, hostname, () =&gt; {
  console.log(<code>Server running at http://${hostname}:${port}/</code>);
});
<code>`</code></p>

After checking out the PR, running the same command again (I've renamed the previous build directory for diffing):

<p>```javascript
const http = require(&#39;http&#39;);</p>
<p>const hostname = &#39;127.0.0.1&#39;;
const port = 3000;</p>
<p>const server = http.createServer((req, res) =&gt; {
  res.statusCode = 200;
  res.setHeader(&#39;Content-Type&#39;, &#39;text/plain&#39;);
  res.end(&#39;Hello World\n&#39;);
});</p>
<p>server.listen(port, hostname, () =&gt; {
  console.log(<code>Server running at http://${hostname}:${port}/</code>);
});
```</p>

I don't know if just something on my local build is borked, so PTAL before merging.

@ghost
Copy link
Author

ghost commented Apr 8, 2019

@fhemberger:This seems a little odd……Here's my index page when running npm run build
image

Here's the result after npm run serve
image

So when you remove all the things in "node_modules" and try to run with npm i, is this still happening?

1) IMPORTANT Fix:Update the 'marked' package, because there's an existing BUG
reported by @rvagg, and this is already fixed in its v0.6.2. For more
Ref:#2119.
Fixture:markedjs/marked#1434.

2) A general upgration of the two packages:
a) require-dir: 1.0.0 => 1.2.0
b) semver: 5.6.0 => 6.0.0 (Fix a technical bug, important)
@ghost
Copy link
Author

ghost commented Apr 8, 2019

Now I've removed the 'package-lock.json' and reinstall all the packages, you can have a try :)

@fhemberger
Copy link
Contributor

Hmm, still the same for me, even with

rm -rf node_modules
rm package-lock.json
npm i
npm run build

🤔

If someone else can confirm the changes are working for them, I'm fine with merging this PR.

@ghost
Copy link
Author

ghost commented Apr 8, 2019

So @fhemberger , when running npm run serve, is everything going with you well? What's up with your local preview page of en/about?

I'm now digging to find what's the matter with that……I'm on Windows 10 (x86, the lastest version of Nodejs, the OS is 17134.648).

@ghost ghost self-requested a review April 8, 2019 10:46
@fhemberger
Copy link
Contributor

@Maledong npm run serve works fine. WTF … 🤔
Seems to be some local oddities, +1 for merging it.

@fhemberger fhemberger merged commit 58f8094 into nodejs:master Apr 8, 2019
fhemberger added a commit that referenced this pull request Apr 8, 2019
When browsing the homepage, the download buttons show direct download links to the detected platform. For Linux, only 64bit builds are supported as of 10.0.0.

Fixes #2172
@Trott
Copy link
Member

Trott commented Apr 8, 2019

The nodejs/node repo stopped using marked (which is RegExp based) started using (I think) remark last year some time. Might be a good idea in this repo too. But also a lot of work. @rubys did most of the heavy-lifting in the core repo, and may have some wisdom to offer.

@fhemberger
Copy link
Contributor

Not sure it's worth it with the upcoming relaunch just around the corner.

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.

2 participants