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

suggestion timer shouldn't require nodejs #1704

Closed
SLKnutson opened this issue Sep 16, 2021 · 8 comments · Fixed by #1714
Closed

suggestion timer shouldn't require nodejs #1704

SLKnutson opened this issue Sep 16, 2021 · 8 comments · Fixed by #1714

Comments

@SLKnutson
Copy link

Is your feature request related to a problem? Please describe.
Hi. We use cspell-trie-lib in the browser for checking spelling and suggesting alternatives. Until very recently, that worked extremely well. After we upgraded (I think to 5.9.0), suggestions stopped working and would instead throw this error (only in the browser).

process is not defined ReferenceError: process is not defined
at createTimer (suggestCollector.js:121)
at Object.collect (suggestCollector.js:40)
at suggest (orthography.js:56)

Describe the solution you'd like
It would be nice to either be able to conditionally disable the timer, or if the timer used a time source that would work in both the browser and nodejs. I'm not sure exactly what that would entail.

Describe alternatives you've considered
It is possible to polyfill that specific node api using https://www.npmjs.com/package/browser-process-hrtime

Additional context
Thanks!

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 16, 2021

@SLKnutson,

Thank you. I had not realized you were using it.

Is there a way for me to try out any fixes?

@SLKnutson
Copy link
Author

Here is a stackblitz that is relatively close to a working example. Stackblitz automatically polyfills process and hrtime, so it's not a perfect example. In a non-polyfilled environment, the entire process variable would be undefined. Thanks!
https://stackblitz.com/edit/typescript-cxsgtp?file=index.ts

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 16, 2021

How are you running it in the browser? Is it Webpacked first? or something else? I'm trying to get a feel for what type of polyfill will work.

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 16, 2021

I took a look at the code and see that I had not "imported" process, I had used the global. Adding the import might fix the issue depending upon how it is packaged.

Jason3S added a commit that referenced this issue Sep 16, 2021
Jason3S added a commit that referenced this issue Sep 16, 2021
* Have a backup timer

Related to #1704
@Jason3S
Copy link
Collaborator

Jason3S commented Sep 16, 2021

I have added the changes.

Please try out ^5.10.0-alpha.5 to see if it works. Since I cannot reproduce your environment, it is difficult to test it out.

@SLKnutson
Copy link
Author

I think that is an improvement because now the code breaks at compile time instead of only when it's called in the browser.

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "process": require.resolve("process/browser") }'
        - install 'process'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "process": false }

Looking at https://github.com/eyas-ranjous/timer-node/blob/master/src/timer.js , it seems like they use Date.now(). That works natively in the browser and in nodejs, and it returns the number of milliseconds since 1970. It doesn't have as much resolution as hrtime (which has nanoseconds), but that probably doesn't matter for this context because the timeout is measured in milliseconds and would probably never be set to a value < 10 milliseconds.

Thanks!

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 17, 2021

Cool. The error makes sense. There is an easy fix on my side.

Jason3S added a commit that referenced this issue Sep 17, 2021
* fix: Use process directly instead of importing it.

Fix: #1704
@github-actions
Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants